You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by selvaganesang <gi...@git.apache.org> on 2018/06/15 23:45:51 UTC
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
GitHub user selvaganesang opened a pull request:
https://github.com/apache/trafodion/pull/1612
[TRAFODION-3110] Refactor LOB access to use the new implementation of…
… HdfsClient
This feature is enabled by default. To disable, set a variable USE_LIBHDFS=1 in
$TRAF_HOME/etc/ms.env and restart the trafodion cluster.
This feature includes the following:
1. Uses single FSDataInputStream for each LOB column in a query as
opposed to the opening the hdfs file for every row.
2. Uses FSDataOutputStream to write the lob data but closes it
immediately to allow concurrent writes to the hdfs file. HDFS supports
a single writer at a time. Need to conform if multiple writes can
be done without the need for RMS lock feature.
3. Improved error messaging that displays the java exception stack to the
end user.
4. LOB worker threads are no longer created
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/selvaganesang/trafodion trafodion_2917_1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1612.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1612
----
commit 52f074af0e866fe82ea4f37eaf90cdc880871851
Author: selvaganesang <se...@...>
Date: 2018-06-15T22:53:04Z
[TRAFODION-3110] Refactor LOB access to use the new implementation of HdfsClient
This feature is enabled by default. To disable, set a variable USE_LIBHDFS=1 in
$TRAF_HOME/etc/ms.env and restart the trafodion cluster.
This feature includes the following:
1. Uses single FSDataInputStream for each LOB column in a query as
opposed to the opening the hdfs file for every row.
2. Uses FSDataOutputStream to write the lob data but closes it
immediately to allow concurrent writes to the hdfs file. HDFS supports
a single writer at a time. Need to conform if multiple writes can
be done without the need for RMS lock feature.
3. Improved error messaging that displays the java exception stack to the
end user.
4. LOB worker threads are no longer created
----
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197894719
--- Diff: core/sql/src/main/java/org/trafodion/sql/HDFSClient.java ---
@@ -359,36 +392,54 @@ int hdfsWrite(byte[] buff) throws IOException
logger_.debug("HDFSClient.hdfsWrite() - output stream created" );
}
outStream_.write(buff);
+ if (outStream_ instanceof FSDataOutputStream)
+ ((FSDataOutputStream)outStream_).hsync();
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsWrite() - bytes written " + buff.length);
return buff.length;
}
- int hdfsRead(ByteBuffer buffer) throws IOException
+ int hdfsRead(long pos, ByteBuffer buffer) throws IOException
{
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsRead() - started" );
if (fsdis_ == null && inStream_ == null ) {
+ try {
codec_ = codecFactory_.getCodec(filepath_);
if (codec_ != null) {
compressed_ = true;
inStream_ = codec_.createInputStream(fs_.open(filepath_));
}
else
fsdis_ = fs_.open(filepath_);
- pos_ = 0;
+ } catch (java.io.FileNotFoundException e) {
+ return 0;
+ }
}
int lenRemain;
--- End diff --
Were these all meant to be int ? For MG and GB writes, will these be enough ?
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198366844
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
@sandhyasun As a contributor involved with LOB interface, can you please let me know what features of LOB are implemented via CLI calls? Within those CLI calls, which of those feature will inovke LOB functions without compilation of SQL statement?
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198243407
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
There is a need to access this CQD value in CLI. Hence, this CQD is converted into SSD. Setting this CQD converts into SSD automatically. See executor/ex_control.cpp. However, the settings of this SSD is such that it is not settable via SSD command. It can be set only via CQD.
I would want USE_LIBHDFS to be settable in the defaults table. Unfortunately, the CQD set in the defaults table is not effective in CLI layer because it is not translated into SSD. I would be fixing it in a separate PR if it is really needed at a future date.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198231385
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
There is already a CQD USE_LIBHDFS, do we also need a session attribute? Also there are some CQDs that can't be set in the ```"_MD_".DEFAULTS``` table. I haven't checked this but hope that USE_LIBHDFS isn't one of them.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198635546
--- Diff: core/sql/executor/HdfsClient_JNI.h ---
@@ -171,7 +175,7 @@ class HdfsClient : public JavaObjectInterface
~HdfsClient();
static HdfsClient *newInstance(NAHeap *heap, ExHdfsScanStats *hdfsStats, HDFS_Client_RetCode &retCode, int hdfsIoByteArraySizeInKB = 0);
static HdfsClient *getInstance();
- static void deleteInstance();
+ static void deleteInstance(HdfsClient *hdfsClient);
--- End diff --
Attempted to make it as non-static method. But it involves the deletion of itself. Hence I decided to keep it as static for better readability.
void HdfsClient::deleteInstance(HdfsClient *hdfsClient)
{
hdfsClient->hdfsClose();
NADELETE(hdfsClient, HdfsClient, hdfsClient->getHeap());
}
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1612
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r195973304
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -2972,28 +3086,8 @@ if (!lobGlobals->isHive() )
void cleanupLOBDataDescFiles(const char *lobHdfsServer,int lobHdfsPort,const char *lobHdfsLoc)
{
- int numExistingFiles=0;
- hdfsFS fs;
- int err = 0;
- fs = hdfsConnect(lobHdfsServer, lobHdfsPort);
- if (fs == NULL)
- return;
- // Get this list of all data and desc files in the lob sotrage location
- hdfsFileInfo *fileInfos = hdfsListDirectory(fs, lobHdfsLoc, &numExistingFiles);
- if (fileInfos == NULL)
- return ;
-
- //Delete each one in a loop
- for (int i = 0; i < numExistingFiles; i++)
- {
- err = hdfsDelete(fs, fileInfos[i].mName, 0);
- }
-
- // *Note* : delete the memory allocated by libhdfs for the file info array
- if (fileInfos)
- {
- hdfsFreeFileInfo(fileInfos, numExistingFiles);
- }
+ HDFS_Client_RetCode hdfsClientRetcode = HdfsClient::hdfsDeletePath(lobHdfsLoc);//ok to ignore error.
--- End diff --
What does lobHdfsLoc point to ? We are supposed to delete only the specific descriptor files belonging to the table we are doing cleanup on.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197952930
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -374,10 +375,9 @@ ex_tcb_private_state *ExHdfsScanTcb::allocatePstates(
Int32 ExHdfsScanTcb::fixup()
{
lobGlob_ = NULL;
-
- ExpLOBinterfaceInit
- (lobGlob_, (NAHeap *)getGlobals()->getDefaultHeap(),getGlobals()->castToExExeStmtGlobals()->getContext(),TRUE, hdfsScanTdb().hostName_,hdfsScanTdb().port_);
-
+ lobGlob_ = ExpLOBoper::initLOBglobal((NAHeap *)getGlobals()->getDefaultHeap(),
+ getGlobals()->castToExExeStmtGlobals()->getContext(),
+ useLibhdfsScan_);
--- End diff --
Not sure what you meant. The threads are started if useLibHdfsScan_ is set true via the above method too
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r195974505
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -7733,26 +7733,12 @@ else
return rc;
//EOD of LOB data file
-
- hdfsFS fs = currContext->getHdfsServerConnection((char*)getLItdb().getHdfsServer(),getLItdb().getHdfsPort());
- if (fs == NULL)
- return LOB_DATA_FILE_OPEN_ERROR;
-
-
snprintf(lobDataFilePath, LOBINFO_MAX_FILE_LEN, "%s/%s", lobLocation, lobDataFile);
- hdfsFile fdData = hdfsOpenFile(fs, lobDataFilePath,O_RDONLY,0,0,0);
- if (!fdData)
- {
- hdfsCloseFile(fs,fdData);
- fdData = NULL;
- return LOB_DATA_FILE_OPEN_ERROR;
- }
- hdfsFileInfo *fInfo = hdfsGetPathInfo(fs, lobDataFilePath);
- if (fInfo)
- lobEOD = fInfo->mSize;
- else
- lobEOD = 0;
-
+ HDFS_Client_RetCode hdfsClientRetcode;
--- End diff --
The toggle is missing here ? We shoudl preserve the old interface here.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r196119679
--- Diff: core/sql/executor/ExExeUtilGet.cpp ---
@@ -7733,26 +7733,12 @@ else
return rc;
//EOD of LOB data file
-
- hdfsFS fs = currContext->getHdfsServerConnection((char*)getLItdb().getHdfsServer(),getLItdb().getHdfsPort());
- if (fs == NULL)
- return LOB_DATA_FILE_OPEN_ERROR;
-
-
snprintf(lobDataFilePath, LOBINFO_MAX_FILE_LEN, "%s/%s", lobLocation, lobDataFile);
- hdfsFile fdData = hdfsOpenFile(fs, lobDataFilePath,O_RDONLY,0,0,0);
- if (!fdData)
- {
- hdfsCloseFile(fs,fdData);
- fdData = NULL;
- return LOB_DATA_FILE_OPEN_ERROR;
- }
- hdfsFileInfo *fInfo = hdfsGetPathInfo(fs, lobDataFilePath);
- if (fInfo)
- lobEOD = fInfo->mSize;
- else
- lobEOD = 0;
-
+ HDFS_Client_RetCode hdfsClientRetcode;
--- End diff --
Yes. At places I have avoided useLibHdfs concept because it is confined and it can be safely replaced without any effect on lob access.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198646970
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
It's fine if the DDL interfaces use the new implementation since we just create or drop. Since you took care of the 2 utility operations UPDATE and EXTRACT, looks like we are covered all the main functionality with this toggle now. Looks good +1 . Can merge as soon as the test passes.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r195975387
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -1827,14 +1934,15 @@ Ex_Lob_Error ExLob::compactLobDataFile(ExLobInMemoryDescChunksEntry *dcArray,Int
lobDataFileSubstr,tmpLobDataFileSubstr, saveLobDataFileSubstr);
lobDebugInfo(logBuf,0,__LINE__,lobTrace_);
- hdfsFS fs = hdfsConnect(hdfsServer_,hdfsPort_);
- if (fs == NULL)
- return LOB_DATA_FILE_OPEN_ERROR;
-
-
- hdfsFile fdData = hdfsOpenFile(fs, lobDataFile_.data(), O_RDONLY, 0, 0,0);
+
--- End diff --
Here too we have missed the toggle ?
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197939561
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -374,10 +375,9 @@ ex_tcb_private_state *ExHdfsScanTcb::allocatePstates(
Int32 ExHdfsScanTcb::fixup()
{
lobGlob_ = NULL;
-
- ExpLOBinterfaceInit
- (lobGlob_, (NAHeap *)getGlobals()->getDefaultHeap(),getGlobals()->castToExExeStmtGlobals()->getContext(),TRUE, hdfsScanTdb().hostName_,hdfsScanTdb().port_);
-
+ lobGlob_ = ExpLOBoper::initLOBglobal((NAHeap *)getGlobals()->getDefaultHeap(),
+ getGlobals()->castToExExeStmtGlobals()->getContext(),
+ useLibhdfsScan_);
--- End diff --
To preserve the old hdfs scan functionality, we may need to preserve the starting of the threads so the scan works correctly when use_lib_hdfs is ON.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r196119643
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -1827,14 +1934,15 @@ Ex_Lob_Error ExLob::compactLobDataFile(ExLobInMemoryDescChunksEntry *dcArray,Int
lobDataFileSubstr,tmpLobDataFileSubstr, saveLobDataFileSubstr);
lobDebugInfo(logBuf,0,__LINE__,lobTrace_);
- hdfsFS fs = hdfsConnect(hdfsServer_,hdfsPort_);
- if (fs == NULL)
- return LOB_DATA_FILE_OPEN_ERROR;
-
-
- hdfsFile fdData = hdfsOpenFile(fs, lobDataFile_.data(), O_RDONLY, 0, 0,0);
+
--- End diff --
Same comment as above
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197957847
--- Diff: core/sql/src/main/java/org/trafodion/sql/HDFSClient.java ---
@@ -359,36 +392,54 @@ int hdfsWrite(byte[] buff) throws IOException
logger_.debug("HDFSClient.hdfsWrite() - output stream created" );
}
outStream_.write(buff);
+ if (outStream_ instanceof FSDataOutputStream)
+ ((FSDataOutputStream)outStream_).hsync();
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsWrite() - bytes written " + buff.length);
return buff.length;
}
--- End diff --
hdfsWrite doesn't close the open and it is assumed to single writer so that we can do streaming write and close it when you are done. hdfsWriteImmediate closes the stream for every write.
hdfsWriteImmediate uses two APIs. One API to get the position where it would be written and another API to do the write. So, it is potentially possible for a different thread and a process to sneak in. However we could do some optimistic concurrency control concepts and avoid locking.
The locking mechanism is not changed. These discussions are just different concepts that could be deployed to improve the locking mechanism and hdfsWriteImmediate method would help to implement the improvements at a later date.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197911343
--- Diff: core/sql/src/main/java/org/trafodion/sql/HDFSClient.java ---
@@ -359,36 +392,54 @@ int hdfsWrite(byte[] buff) throws IOException
logger_.debug("HDFSClient.hdfsWrite() - output stream created" );
}
outStream_.write(buff);
+ if (outStream_ instanceof FSDataOutputStream)
+ ((FSDataOutputStream)outStream_).hsync();
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsWrite() - bytes written " + buff.length);
return buff.length;
}
--- End diff --
In case of hdfsWriteImmediate we are returning the offset of the file after the write has been completed. But in the case of hdfsWrite, we seem to return the length of the write operation ?
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197980387
--- Diff: core/sql/executor/ExHdfsScan.cpp ---
@@ -374,10 +375,9 @@ ex_tcb_private_state *ExHdfsScanTcb::allocatePstates(
Int32 ExHdfsScanTcb::fixup()
{
lobGlob_ = NULL;
-
- ExpLOBinterfaceInit
- (lobGlob_, (NAHeap *)getGlobals()->getDefaultHeap(),getGlobals()->castToExExeStmtGlobals()->getContext(),TRUE, hdfsScanTdb().hostName_,hdfsScanTdb().port_);
-
+ lobGlob_ = ExpLOBoper::initLOBglobal((NAHeap *)getGlobals()->getDefaultHeap(),
+ getGlobals()->castToExExeStmtGlobals()->getContext(),
+ useLibhdfsScan_);
--- End diff --
Yes. Found the problem isHiveRead is not passed through the new methods. Fixing it to ensure that use_libhdfs 'on' works for hdfs scan
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198282309
--- Diff: core/sql/executor/HdfsClient_JNI.h ---
@@ -171,7 +175,7 @@ class HdfsClient : public JavaObjectInterface
~HdfsClient();
static HdfsClient *newInstance(NAHeap *heap, ExHdfsScanStats *hdfsStats, HDFS_Client_RetCode &retCode, int hdfsIoByteArraySizeInKB = 0);
static HdfsClient *getInstance();
- static void deleteInstance();
+ static void deleteInstance(HdfsClient *hdfsClient);
--- End diff --
Yes. I should have done. I guess I got influenced by the existing method. Will change it when I get a chance
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198292447
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
Yes, fixing all those CQDs to allow entries in the system defaults table would be nice. But, that will run into some design issues: CQDs affect compilation, session attributes affect the runtime, so there is a mismatch in the scope. I would prefer a solution that respects those designs. Would having a flag word in the statement globals help?
You say that the USE_LIBHDFS CQD is just a fallback, but that is exactly what won't work if we don't support the system defaults table. Let's say a user would have to re-enable libhdfs because of some problem. If they can't do that in the system defaults table, they would have to change all their affected programs, which is not going to make them very happy. If it's just for fallback in case of problems, I think an environment variable will be better for users. I know that the CQD is of course better for our own testing.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197914060
--- Diff: core/sql/src/main/java/org/trafodion/sql/HDFSClient.java ---
@@ -359,36 +392,54 @@ int hdfsWrite(byte[] buff) throws IOException
logger_.debug("HDFSClient.hdfsWrite() - output stream created" );
}
outStream_.write(buff);
+ if (outStream_ instanceof FSDataOutputStream)
+ ((FSDataOutputStream)outStream_).hsync();
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsWrite() - bytes written " + buff.length);
return buff.length;
}
--- End diff --
Yes. . HdfsWriteImmediate used by LOB access. It needs the offset where the write occurred. We could prevent the lob locking by doing write to hdfs file first and then use this offset to write the chunk offset data to Trafodion desc chunk table. This can avoid locking with some kind of smartness built into this java method or at least limit the locking duration to this method alone.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198287745
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
I found that it wasn't possible to do like CQD LOB_MAX_SIZE because this setting is needed in ExLOBGlobals constructor. The best way is to transform it into SSD, if the settings is needed in CLI layer. Then I realized that CQD is effective but SSD isn't effective if the CQD is set in defaults table. Currently if you set this CQD to 'ON' in defaults tables, the portions of the code that created ExLobGlobals from CLI layer still uses newer implementation.
USE_LIBHDFS is just fall back option till such time the new implementation is tested. Then, I would think the code and the CQD can be retired. I haven't it figured out how much impact is not to use USE_LIBHDFS settings from the CLI layer when we need to fallback. I have run core/TEST130 with CQD USE_LIBHDFS 'ON' in defaults table.
Because it is a generic issue of not transforming to SSD from CQD when it is defaults table, I thought that I will handle it via another PR. In addition, this would require changes to an area that I am not yet comfortable with.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197898558
--- Diff: core/sql/src/main/java/org/trafodion/sql/HDFSClient.java ---
@@ -359,36 +392,54 @@ int hdfsWrite(byte[] buff) throws IOException
logger_.debug("HDFSClient.hdfsWrite() - output stream created" );
}
outStream_.write(buff);
+ if (outStream_ instanceof FSDataOutputStream)
+ ((FSDataOutputStream)outStream_).hsync();
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsWrite() - bytes written " + buff.length);
return buff.length;
}
- int hdfsRead(ByteBuffer buffer) throws IOException
+ int hdfsRead(long pos, ByteBuffer buffer) throws IOException
{
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsRead() - started" );
if (fsdis_ == null && inStream_ == null ) {
+ try {
codec_ = codecFactory_.getCodec(filepath_);
if (codec_ != null) {
compressed_ = true;
inStream_ = codec_.createInputStream(fs_.open(filepath_));
}
else
fsdis_ = fs_.open(filepath_);
- pos_ = 0;
+ } catch (java.io.FileNotFoundException e) {
+ return 0;
+ }
}
int lenRemain;
--- End diff --
Yes. The chunking should happen in the JNI side.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198342357
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
+1
While I do think the CQD is a problem, given that this is a temporary situation we should be able to live with the current state of things. If all fails we may need to produce a code patch.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198638476
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
The update and extract LOB commands were not adhering to USE_LIBHDFS settings. This has been fixed in commit ee44ad9. However, all the DDL lob functions executed via the CLI call SQL_EXEC_LOBddlInterface will not adhere to USE_LIBHDFS settings if the CQD is set in defaults table. Instead the CQD should be set via DCS profile till we fix the issue of transforming CQD into SSD for the CQDs in defaults table. Because it is is DDLinterface, it should be ok even if it couldn't switch to older implementation.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197893132
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -2161,10 +2267,37 @@ Ex_Lob_Error ExLob::readDataToMem(char *memAddr,
}
-
+ } // useLibHdfs_
if (!multipleChunks)
{
+ if (! useLibHdfs_) {
+ HDFS_Client_RetCode hdfsClientRetcode;
+ Int32 readLen;
--- End diff --
Shouldn't it use Int64 for readLen/ It's being copied into operLen.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198346030
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
I am confused by this now. Are we saying if we set the CQD USE_LIBHDFS to ON, the code today will not revert to the old behavior ? Given several POCs are using this feature and usually under a time crunch, it will not be practical provide a patch. I think we need a mechanism to revert to the old interface for 2 reasons. We also need to run performance tests with very large LOBs. China team is waiting for this and need a way to turn this feature off and on for sure. Maybe envvar is the way to go if this CQD/SSD is only partial.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197942792
--- Diff: core/sql/src/main/java/org/trafodion/sql/HDFSClient.java ---
@@ -359,36 +392,54 @@ int hdfsWrite(byte[] buff) throws IOException
logger_.debug("HDFSClient.hdfsWrite() - output stream created" );
}
outStream_.write(buff);
+ if (outStream_ instanceof FSDataOutputStream)
+ ((FSDataOutputStream)outStream_).hsync();
if (logger_.isDebugEnabled())
logger_.debug("HDFSClient.hdfsWrite() - bytes written " + buff.length);
return buff.length;
}
--- End diff --
Ok. I see one placce where hdfsWrite is also called. But int hat case I suppose we don't need to the EOD. Eitherway, what is the difference between hdfsWrite and hdfsWriteImmediate ? Also while hdfsWriteImmediate is coccuring , can another threadprocess write to the same hdfsfile or is the entire hdfsWriteImmediate done under a lock ?
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198222573
--- Diff: core/sql/executor/HdfsClient_JNI.h ---
@@ -171,7 +175,7 @@ class HdfsClient : public JavaObjectInterface
~HdfsClient();
static HdfsClient *newInstance(NAHeap *heap, ExHdfsScanStats *hdfsStats, HDFS_Client_RetCode &retCode, int hdfsIoByteArraySizeInKB = 0);
static HdfsClient *getInstance();
- static void deleteInstance();
+ static void deleteInstance(HdfsClient *hdfsClient);
--- End diff --
Wouldn't it be more straightforward to just make this a non-static method with no arguments?
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198377670
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
Do you means the different LOB functions/syntax ? LOB itself is considered one single feature.
Different LOB operations are implemented as functions eg filetolob(), buffertolob(), stringtolob() which get compiled as part of a SQL DML statement. These get evaluated at runtime .
And some LOB operations are implemented as Executor utility operations eg : UPDATE LOB and EXTRACT LOB.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by zellerh <gi...@git.apache.org>.
Github user zellerh commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198246599
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
Thanks for the explanation. Don't you think that it will be a requirement to make this work with the defaults table? Maybe an environment variable would be a better choice if the defaults table doesn't work?
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r198262065
--- Diff: core/sql/cli/SessionDefaults.cpp ---
@@ -108,6 +108,7 @@ static const SessionDefaults::SessionDefaultMap sessionDefaultMap[] =
SDEntry(SessionDefaults::SCHEMA, SCHEMA, SessionDefaults::SDT_ASCII, TRUE, TRUE, FALSE, FALSE),
SDEntry(SessionDefaults::STATISTICS_VIEW_TYPE, STATISTICS_VIEW_TYPE, SessionDefaults::SDT_ASCII, FALSE, FALSE, TRUE, TRUE),
SDEntry(SessionDefaults::SUSPEND_LOGGING, SUSPEND_LOGGING, SessionDefaults::SDT_BOOLEAN, FALSE, FALSE, TRUE, FALSE),
+ SDEntry(SessionDefaults::USE_LIBHDFS, USE_LIBHDFS, SessionDefaults::SDT_BOOLEAN, TRUE, TRUE, FALSE, FALSE),
--- End diff --
The CQD could have been passed in just like we pass in LOB_MAX_SIZE and LOB_MAX_MEM_CHUNK size but that makes the code a bit messy and Selva will need to change way too many interfaces. The CQD as SSD is a good choice for this particular case since it's a "setting". It's better not to have it as an envvar since switching back and forth will make it more difficult.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r196208968
--- Diff: core/sql/exp/ExpLOBaccess.cpp ---
@@ -2972,28 +3086,8 @@ if (!lobGlobals->isHive() )
void cleanupLOBDataDescFiles(const char *lobHdfsServer,int lobHdfsPort,const char *lobHdfsLoc)
{
- int numExistingFiles=0;
- hdfsFS fs;
- int err = 0;
- fs = hdfsConnect(lobHdfsServer, lobHdfsPort);
- if (fs == NULL)
- return;
- // Get this list of all data and desc files in the lob sotrage location
- hdfsFileInfo *fileInfos = hdfsListDirectory(fs, lobHdfsLoc, &numExistingFiles);
- if (fileInfos == NULL)
- return ;
-
- //Delete each one in a loop
- for (int i = 0; i < numExistingFiles; i++)
- {
- err = hdfsDelete(fs, fileInfos[i].mName, 0);
- }
-
- // *Note* : delete the memory allocated by libhdfs for the file info array
- if (fileInfos)
- {
- hdfsFreeFileInfo(fileInfos, numExistingFiles);
- }
+ HDFS_Client_RetCode hdfsClientRetcode = HdfsClient::hdfsDeletePath(lobHdfsLoc);//ok to ignore error.
--- End diff --
Checked again and this is ok . The cleanupLOBDataDescFiles is called form sqlcomp during ::dropSeabaseMD. That's used when we are doing a complete cleanup of all trafodion objects so it's ok to drop all the LOB data files too.
---
[GitHub] trafodion pull request #1612: [TRAFODION-3110] Refactor LOB access to use th...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1612#discussion_r197893780
--- Diff: core/sql/executor/HdfsClient_JNI.h ---
@@ -182,13 +186,16 @@ class HdfsClient : public JavaObjectInterface
HDFS_Client_RetCode init();
HDFS_Client_RetCode hdfsCreate(const char* path, NABoolean overwrite, NABoolean compress);
HDFS_Client_RetCode hdfsOpen(const char* path, NABoolean compress);
- Int32 hdfsWrite(const char* data, Int64 size, HDFS_Client_RetCode &hdfsClientRetcode);
- Int32 hdfsRead(const char* data, Int64 size, HDFS_Client_RetCode &hdfsClientRetcode);
+ Int64 hdfsSize(HDFS_Client_RetCode &hdfsClientRetcode);
+ Int32 hdfsWrite(const char* data, Int64 size, HDFS_Client_RetCode &hdfsClientRetcode, int maxChunkSize = 0);
--- End diff --
maxChunkSize is passed in as Int64 from caller. Here it's declared as int.
---