You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by sureshsubbiah <gi...@git.apache.org> on 2015/09/20 19:11:36 UTC
[GitHub] incubator-trafodion pull request: AFODION-1492] Allow specificatio...
GitHub user sureshsubbiah opened a pull request:
https://github.com/apache/incubator-trafodion/pull/88
AFODION-1492] Allow specification of MaxHeapSize for JVM started by UDR server
This is a fix for the second of the two issues described in this JIRA. The first
has been addressed by narendra in a separate checkin.
In this checkin we allow multiple methods to specify JVM startup options for the
UDR server process. There are three methods applied in this order with the last
specification for each property superceeding previous specifications.
1. A default default of 512MB for UDR JVM's max heap size
2. Specify any desired property including -Xmx in a new configuration file
3. Specify any desired property using TRAF_UDR_JAVA_OPTIONS env variable
The new configuration file is at $MY_SQROT/conf/trafodion.udr.config. It has
detailed comments on how properties should be specified in the file. The location
(including name) of the configuration file can be overridden with the env variable
TRAFUDRCFG.
The TRAF_UDR_JAVA_OPTIONS env variable must be specified in $MY_SQROOT/etc/ms.env
for it to be visible to the UDR server. Multiple properties can be separated by a
'\t' (TAB) chracater or the '\n' (newline) character
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sureshsubbiah/incubator-trafodion jira_bugs3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-trafodion/pull/88.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 #88
----
commit 00dd462e66c1c44f676465c4dee4a5cbb407d0f6
Author: Suresh Subbiah <su...@apache.org>
Date: 2015-09-20T17:08:19Z
AFODION-1492] Allow specification of MaxHeapSize for JVM started by UDR server
This is a fix for the second of the two issues described in this JIRA. The first
has been addressed by narendra in a separate checkin.
In this checkin we allow multiple methods to specify JVM startup options for the
UDR server process. There are three methods applied in this order with the last
specification for each property superceeding previous specifications.
1. A default default of 512MB for UDR JVM's max heap size
2. Specify any desired property including -Xmx in a new configuration file
3. Specify any desired property using TRAF_UDR_JAVA_OPTIONS env variable
The new configuration file is at $MY_SQROT/conf/trafodion.udr.config. It has
detailed comments on how properties should be specified in the file. The location
(including name) of the configuration file can be overridden with the env variable
TRAFUDRCFG.
The TRAF_UDR_JAVA_OPTIONS env variable must be specified in $MY_SQROOT/etc/ms.env
for it to be visible to the UDR server. Multiple properties can be separated by a
'\t' (TAB) chracater or the '\n' (newline) character
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1492] Allow specifica...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-trafodion/pull/88
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1492] Allow specifica...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/88#discussion_r39934296
--- Diff: core/sql/udrserv/UdrCfgParser.cpp ---
@@ -44,22 +44,15 @@ NABoolean UdrCfgParser::cfgFileIsOpen(NAString &errorText)
NABoolean envFound = FALSE;
- if(cfgFileName = getenv("MXUDRCFG"))
+ if(cfgFileName = getenv("TRAFUDRCFG"))
{
envFound = TRUE;
- UDR_DEBUG1("UdrCfgParser(): MXUDRCFG cfgFileName is %s", cfgFileName);
+ UDR_DEBUG1("UdrCfgParser(): TRAFUDRCFG cfgFileName is %s", cfgFileName);
}
else
{
- NAString s("c:/tdm_sql/udr/mxudrcfg");
- char installdir[1024];
- Lng32 resultlength = 0;
- Int32 res = ComRtGetInstallDir(installdir, 1024, &resultlength);
- if (res == 0)
- {
- s = installdir;
- s += "/mxudrcfg";
- }
+ NAString s(getenv("MY_SQROOT"));
--- End diff --
What does NAString ctor do if passed a null pointer? (That is, what happens if $MY_SQROOT is not defined? Unlikely, I know.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1492] Allow specifica...
Posted by sureshsubbiah <gi...@git.apache.org>.
Github user sureshsubbiah commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/88#discussion_r39982347
--- Diff: core/sql/udrserv/udrserv.cpp ---
@@ -170,24 +170,28 @@ static Int32 invokeUdrMethod(const char *method,
// LCOV_EXCL_START
// Dead Code
// These methods are not used, and the interface has not been tested for a long time.
-// Andy is of the opinion that we might want to retire them
+// We might want to retire them
static Int32 processCommandsFromFile(const char *filename, UdrGlobals &glob);
static Int32 processSingleCommandFromFile(FILE *f, UdrGlobals &glob);
// LCOV_EXCL_STOP
+// Changed the default to 512 to limit java heap size used by SQL processes.
+// Keep this define in sync with executor/JavaObjectInterface.cpp
+#define DEFAULT_JVM_MAX_HEAP_SIZE 512
--- End diff --
There are only a few shared header files between udr process and executor. Udr does not have the cli. Rather than put this in a basic header file like Platform.h or NAMemory.h I thought this would be better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1492] Allow specifica...
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/88#discussion_r39934328
--- Diff: core/sql/udrserv/udrserv.cpp ---
@@ -170,24 +170,28 @@ static Int32 invokeUdrMethod(const char *method,
// LCOV_EXCL_START
// Dead Code
// These methods are not used, and the interface has not been tested for a long time.
-// Andy is of the opinion that we might want to retire them
+// We might want to retire them
static Int32 processCommandsFromFile(const char *filename, UdrGlobals &glob);
static Int32 processSingleCommandFromFile(FILE *f, UdrGlobals &glob);
// LCOV_EXCL_STOP
+// Changed the default to 512 to limit java heap size used by SQL processes.
+// Keep this define in sync with executor/JavaObjectInterface.cpp
+#define DEFAULT_JVM_MAX_HEAP_SIZE 512
--- End diff --
Not a big deal, but would it be possible to move the define to a shared header file to avoid the need to manually sync?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-trafodion pull request: [TRAFODION-1492] Allow specifica...
Posted by sureshsubbiah <gi...@git.apache.org>.
Github user sureshsubbiah commented on a diff in the pull request:
https://github.com/apache/incubator-trafodion/pull/88#discussion_r39982124
--- Diff: core/sql/udrserv/UdrCfgParser.cpp ---
@@ -44,22 +44,15 @@ NABoolean UdrCfgParser::cfgFileIsOpen(NAString &errorText)
NABoolean envFound = FALSE;
- if(cfgFileName = getenv("MXUDRCFG"))
+ if(cfgFileName = getenv("TRAFUDRCFG"))
{
envFound = TRUE;
- UDR_DEBUG1("UdrCfgParser(): MXUDRCFG cfgFileName is %s", cfgFileName);
+ UDR_DEBUG1("UdrCfgParser(): TRAFUDRCFG cfgFileName is %s", cfgFileName);
}
else
{
- NAString s("c:/tdm_sql/udr/mxudrcfg");
- char installdir[1024];
- Lng32 resultlength = 0;
- Int32 res = ComRtGetInstallDir(installdir, 1024, &resultlength);
- if (res == 0)
- {
- s = installdir;
- s += "/mxudrcfg";
- }
+ NAString s(getenv("MY_SQROOT"));
--- End diff --
We are using Facebook's string implementation, which is defined at https://github.com/facebook/folly/blob/master/folly/FBString.h
I think it will be an empty string. I could not make that determination from FB source code. Will try it on code with a debugger.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---