You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by sandhyasun <gi...@git.apache.org> on 2018/01/18 16:23:36 UTC
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
GitHub user sandhyasun opened a pull request:
https://github.com/apache/trafodion/pull/1405
[TRAFODION-2873] Removed usage of system new from LobGlobals and other classes
ExpOBInterfaceInit will take a parent heap and derive a lobHeap from it. ExLobGlobals will get created from this heap and store this heap pointer for use by other classes.
ExpLOBinterfaceCleanup destroys the LOB globals and will utilize the heap pointer stored within it.
In cli/Cli.cpp , Removed the initialization/destruction of LOB glabals from the ::createLOB interface and moved it out before the interface is called to be consistent with other LOB API usage.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sandhyasun/trafodion traf_lob_global_fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1405.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 #1405
----
commit 0da83d525d7e25b546181b0360cf3f3d9d422721
Author: Sandhya Sundaresan <sa...@...>
Date: 2018-01-18T16:17:41Z
Removed usage of system new and moved allocation of LOB globals to be derived from a heap. Moved xLObHdfsRequest also to be derived from a heap
----
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162492297
--- Diff: core/sql/cli/Cli.cpp ---
@@ -9726,10 +9742,26 @@ Lng32 SQLCLI_LOBddlInterface
//lob data files. Note that if there is an error in the drop of the
//descriptor tables above , the transaction will restore each of the
//above tables .
+ //Initialize LOB interface
+
+ Int32 rc= ExpLOBoper::initLOBglobal(exLobGlob,currContext.exHeap(),&currContext,hdfsServer,hdfsPort);
+ if (rc)
+ {
+ {
--- End diff --
Same comment here
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162492341
--- Diff: core/sql/cli/Cli.cpp ---
@@ -9765,12 +9797,27 @@ Lng32 SQLCLI_LOBddlInterface
goto error_return;
}
-
+ //Initialize LOB interface
+
+ Int32 rc= ExpLOBoper::initLOBglobal(exLobGlob,currContext.exHeap(),&currContext,hdfsServer,hdfsPort);
+ if (rc)
+ {
+ {
--- End diff --
And here
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162546998
--- Diff: core/sql/exp/ExpLOBprocess.cpp ---
@@ -575,7 +575,7 @@ Lng32 main(Lng32 argc, char *argv[])
// setup log4cxx
QRLogger::initLog4cxx(QRLogger::QRL_LOB);
// initialize lob globals
- lobGlobals = new ExLobGlobals();
+ lobGlobals = new ExLobGlobals(NULL);
--- End diff --
The ExpLOBprocess.cpp is dormant code for now. If we enable it and create a LOB process to offload some operations like GC etc, we will need to create and setup globals , send messages form the master process to it etc . For now I just had to ensure it compiled.
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162542021
--- Diff: core/sql/exp/ExpLOBprocess.cpp ---
@@ -575,7 +575,7 @@ Lng32 main(Lng32 argc, char *argv[])
// setup log4cxx
QRLogger::initLog4cxx(QRLogger::QRL_LOB);
// initialize lob globals
- lobGlobals = new ExLobGlobals();
+ lobGlobals = new ExLobGlobals(NULL);
--- End diff --
Thanks for avoiding the default value for the param. But, I think sending heap as NULL might make this program to fail. Have you tested this program? If you have tested this program already, then this PR is ready to be merged.
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162491703
--- Diff: core/sql/exp/ExpLOBaccess.h ---
@@ -589,7 +589,7 @@ class ExLobGlobals
{
public :
- ExLobGlobals();
+ ExLobGlobals(NAHeap *lobHeap=NULL);
--- End diff --
To @selvaganesang's point, perhaps we should leave this without a default, and then code NULL explicitly in ExpLobProcess.cpp?
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1405
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162492113
--- Diff: core/sql/cli/Cli.cpp ---
@@ -9611,12 +9611,28 @@ Lng32 SQLCLI_LOBddlInterface
} // for
+ //Initialize LOB interface
+
+ Int32 rc= ExpLOBoper::initLOBglobal(exLobGlob,currContext.exHeap(),&currContext,hdfsServer,hdfsPort);
+ if (rc)
+ {
+ {
--- End diff --
It doesn't hurt anything, but I'm curious: Why two sets of braces? Looks like one would do?
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162448037
--- Diff: core/sql/exp/ExpLOBaccess.h ---
@@ -589,7 +589,7 @@ class ExLobGlobals
{
public :
- ExLobGlobals();
+ ExLobGlobals(NAHeap *lobHeap=NULL);
--- End diff --
Yes this was only done to be able to compile the call in ExpLobProcess.cpp. Currently this code is unused and is kept around in case we need to offload any processing to another process. This code, invokes LobGlobals and there is no heap available. So I had to make it default just for this unused code. WIll put a comment that it shoudl always pass a heap. In anycase a caller cannot initialize the LobGlobals without passing in a caller's heap - i.e the only way to initialize is by calling ExpLOBinterfaceInit or ExpLOBoper::initLobGlobal. Both require a heap to get passed in.
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by sandhyasun <gi...@git.apache.org>.
Github user sandhyasun commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162494316
--- Diff: core/sql/exp/ExpLOBaccess.h ---
@@ -589,7 +589,7 @@ class ExLobGlobals
{
public :
- ExLobGlobals();
+ ExLobGlobals(NAHeap *lobHeap=NULL);
--- End diff --
Sure that's another way. Will do that.
---
[GitHub] trafodion pull request #1405: [TRAFODION-2873] Removed usage of system new f...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1405#discussion_r162442989
--- Diff: core/sql/exp/ExpLOBaccess.h ---
@@ -589,7 +589,7 @@ class ExLobGlobals
{
public :
- ExLobGlobals();
+ ExLobGlobals(NAHeap *lobHeap=NULL);
--- End diff --
It is better to avoid the default value for this parameter.
---