You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by CoderSong2015 <gi...@git.apache.org> on 2018/03/20 07:53:50 UTC
[GitHub] trafodion pull request #1487: Trafodion keepalive support
GitHub user CoderSong2015 opened a pull request:
https://github.com/apache/trafodion/pull/1487
Trafodion keepalive support
Keepalive could be configured by modifying file src/main/java/org/trafodion/dcs/Constants.java or src/main/resources/dcs-default.xml
Modify variable DCS_SERVER_PROGRAM_KEEPALIVE_OPT;
It's a string with parameter ENABLE IDLETIME INTERTIME RETRYCNT (Seconds)
ENABLE=enable/unenable/default ,IDLETIME= number ,INTERTIME=number,RETRYCNT=number
e.g:
ENABLE=enable,IDLETIME=300,INTERTIME=5,RETRYCNT=3
Default value is 1,3600,3,20(Only effective when value configured is set incorrectly)
The value will be read in when mxosrvr start. Mxosrvr will set the socket after getting a connection.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/CoderSong2015/Apache-Trafodion keepalive
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1487.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 #1487
----
commit 06c9a95d34203b8476cb96cea07e86daff1724f7
Author: Haolin.song <40...@...>
Date: 2018-03-20T15:31:31Z
Trafodion keepalive support
Keepalive could be configured by modifying file src/main/java/org/trafodion/dcs/Constants.java or src/main/resources/dcs-default.xml
Modify variable DCS_SERVER_PROGRAM_KEEPALIVE_OPT;
It's a string with parameter ENABLE IDLETIME INTERTIME RETRYCNT (Seconds)
ENABLE=enable/unenable/default ,IDLETIME= number ,INTERTIME=number,RETRYCNT=number
e.g:
ENABLE=enable,IDLETIME=300,INTERTIME=5,RETRYCNT=3
Default value is 1,3600,3,20(Only effective when value configured is set incorrectly)
The value will be read in when mxosrvr start. Mxosrvr will set the socket after getting a connection.
----
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by CoderSong2015 <gi...@git.apache.org>.
Github user CoderSong2015 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r176968469
--- Diff: core/conn/odbc/src/odbc/Common/Global.h ---
@@ -935,7 +941,10 @@ typedef struct _SRVR_GLOBAL_Def
bzero(m_ProcName,sizeof(m_ProcName));
m_bNewConnection = false;
m_bNewService = false;
-
+ bzero(clientKeepaliveStatus, sizeof(clientKeepaliveStatus));
--- End diff --
That because old code uses tab which equals 8 blanks but i use 4 blanks directly（so the code aligned on my vim since I set one tab equals 4 blanks.） I will modify it to keep the indentation consistent, on github.
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by CoderSong2015 <gi...@git.apache.org>.
Github user CoderSong2015 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r176970193
--- Diff: core/conn/odbc/src/odbc/nsksrvr/SrvrMain.cpp ---
@@ -1427,7 +1437,59 @@ BOOL getInitParamSrvr(int argc, char *argv[], SRVR_INIT_PARAM_Def &initParam, ch
argEmpty = TRUE;
break;
}
- }
+ }else
+ if (strcmp(arg, "-TCPKEEPALIVESTATUS") == 0){
+ if (++count < argc && argv[count][0] != '-')
+ {
+ if (strlen(argv[count]) < sizeof(keepaliveStatus) - 1)
+ {
+ memset(keepaliveStatus, 0, sizeof(keepaliveStatus));
+ strncpy(keepaliveStatus, argv[count], sizeof(keepaliveStatus));
+ }
+ else
+ {
+ argWrong = TRUE;
+ }
+ }
+ else
+ {
+ argEmpty = TRUE;
+ break;
+ }
+ }else
+ if (strcmp(arg, "-TCPKEEPALIVEIDLETIME") == 0){
+ if (++count < argc )
+ {
+ keepaliveIdletime = atoi(argv[count]);
--- End diff --
I just used the code above as a reference. I think I need to modify the all relevant codes to keep the robustness. Thanks very much for your all suggestions.
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r176477432
--- Diff: core/conn/odbc/src/odbc/nsksrvr/Interface/Listener_srvr.cpp ---
@@ -81,4 +81,46 @@ void CNSKListenerSrvr::TCP_PROCESSNAME_PORT(FILE* fp)
fprintf(fp,"<==========TCP/PORT (%s/%d)==========>\n",m_TcpProcessName, m_port);
}
+void CNSKListenerSrvr::TCP_SetKeepalive(int socketnum,
+ char *keepaliveStatus,
+ int idleTime,
+ int intervalTime,
+ int retryCount)
+{
+ //all need to be configured
+ if(NULL == keepaliveStatus){
+ return;
+ }
+ if(0 == strcmp(keepaliveStatus,"default")){
+ keepaliveOpt.isKeepalive = DEFAULT_KEEPALIVE;
+ keepaliveOpt.keepaliveIdle = DEFAULT_KEEPALIVE_TIMESEC;
+ keepaliveOpt.keepaliveInterval = DEFAULT_KEEPALIVE_INTVL;
+ keepaliveOpt.keepCount = DEFAULT_KEEPALIVE_COUNT;
+ }else
+ if(0 == strcmp(keepaliveStatus,"unenable")){
+ keepaliveOpt.isKeepalive = 0;
+ keepaliveOpt.keepaliveIdle = DEFAULT_KEEPALIVE_TIMESEC;
+ keepaliveOpt.keepaliveInterval = DEFAULT_KEEPALIVE_INTVL;
+ keepaliveOpt.keepCount = DEFAULT_KEEPALIVE_COUNT;
+ }else
+ if(0 == strcmp(keepaliveStatus, "enable")){
+ keepaliveOpt.isKeepalive = 1;
+ keepaliveOpt.keepaliveIdle = idleTime;
+ keepaliveOpt.keepaliveInterval = intervalTime;
+ keepaliveOpt.keepCount = retryCount;
+
+ }else{
+ keepaliveOpt.isKeepalive = 0;
--- End diff --
Why are the other members (keepaliveIdle and so on) not set in this code path? (I assume it matters since we set them in the "unenable" code path.)
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r176479189
--- Diff: core/conn/odbc/src/odbc/nsksrvr/SrvrMain.cpp ---
@@ -1427,7 +1437,59 @@ BOOL getInitParamSrvr(int argc, char *argv[], SRVR_INIT_PARAM_Def &initParam, ch
argEmpty = TRUE;
break;
}
- }
+ }else
+ if (strcmp(arg, "-TCPKEEPALIVESTATUS") == 0){
+ if (++count < argc && argv[count][0] != '-')
+ {
+ if (strlen(argv[count]) < sizeof(keepaliveStatus) - 1)
+ {
+ memset(keepaliveStatus, 0, sizeof(keepaliveStatus));
+ strncpy(keepaliveStatus, argv[count], sizeof(keepaliveStatus));
--- End diff --
I think you want "sizeof(keepaliveStatus)-1" here. If argv[count] has a strlen longer than keepaliveStatus, the first sizeof(keepaliveStatus) bytes will be copied here, overwriting all of the bytes previously set to null. (I don't know what we do for other parameters. Maybe we tolerate this situation today.)
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by CoderSong2015 <gi...@git.apache.org>.
Github user CoderSong2015 closed the pull request at:
https://github.com/apache/trafodion/pull/1487
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r176478042
--- Diff: core/conn/odbc/src/odbc/Common/Global.h ---
@@ -935,7 +941,10 @@ typedef struct _SRVR_GLOBAL_Def
bzero(m_ProcName,sizeof(m_ProcName));
m_bNewConnection = false;
m_bNewService = false;
-
+ bzero(clientKeepaliveStatus, sizeof(clientKeepaliveStatus));
--- End diff --
Minor point: If we could keep the indentation consistent, that will make the code easier to read in the future. (Perhaps the new code uses tabs and the old code doesn't? Or maybe the old code uses tabs but the new code doesn't? In the past, we've tried to avoid tabs because they lead to these kinds of problems but we have not been successful in keeping them out.)
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by CoderSong2015 <gi...@git.apache.org>.
Github user CoderSong2015 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r176969286
--- Diff: core/conn/odbc/src/odbc/nsksrvr/Interface/Listener_srvr.cpp ---
@@ -81,4 +81,46 @@ void CNSKListenerSrvr::TCP_PROCESSNAME_PORT(FILE* fp)
fprintf(fp,"<==========TCP/PORT (%s/%d)==========>\n",m_TcpProcessName, m_port);
}
+void CNSKListenerSrvr::TCP_SetKeepalive(int socketnum,
+ char *keepaliveStatus,
+ int idleTime,
+ int intervalTime,
+ int retryCount)
+{
+ //all need to be configured
+ if(NULL == keepaliveStatus){
+ return;
+ }
+ if(0 == strcmp(keepaliveStatus,"default")){
+ keepaliveOpt.isKeepalive = DEFAULT_KEEPALIVE;
+ keepaliveOpt.keepaliveIdle = DEFAULT_KEEPALIVE_TIMESEC;
+ keepaliveOpt.keepaliveInterval = DEFAULT_KEEPALIVE_INTVL;
+ keepaliveOpt.keepCount = DEFAULT_KEEPALIVE_COUNT;
+ }else
+ if(0 == strcmp(keepaliveStatus,"unenable")){
+ keepaliveOpt.isKeepalive = 0;
+ keepaliveOpt.keepaliveIdle = DEFAULT_KEEPALIVE_TIMESEC;
+ keepaliveOpt.keepaliveInterval = DEFAULT_KEEPALIVE_INTVL;
+ keepaliveOpt.keepCount = DEFAULT_KEEPALIVE_COUNT;
+ }else
+ if(0 == strcmp(keepaliveStatus, "enable")){
+ keepaliveOpt.isKeepalive = 1;
+ keepaliveOpt.keepaliveIdle = idleTime;
+ keepaliveOpt.keepaliveInterval = intervalTime;
+ keepaliveOpt.keepCount = retryCount;
+
+ }else{
+ keepaliveOpt.isKeepalive = 0;
--- End diff --
I know what you mean. Although Other members will not work when the keepalive itself is turned off, I should set them in that code path to keep the code style consistent， like above, to avoid misunderstandings.
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by CoderSong2015 <gi...@git.apache.org>.
Github user CoderSong2015 closed the pull request at:
https://github.com/apache/trafodion/pull/1487
---
[GitHub] trafodion pull request #1487: Trafodion keepalive support
Posted by CoderSong2015 <gi...@git.apache.org>.
Github user CoderSong2015 commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r175973570
--- Diff: dcs/src/main/java/org/trafodion/dcs/Constants.java ---
@@ -92,7 +92,7 @@
public static final String DCS_SERVER_USER_PROGRAM_COMMAND = "dcs.server.user.program.command";
/** Default value for DCS server user program command */
- public static final String DEFAULT_DCS_SERVER_USER_PROGRAM_COMMAND = "cd ${dcs.user.program.home};. ./sqenv.sh;mxosrvr -ZKHOST -RZ -ZKPNODE -CNGTO -ZKSTO -EADSCO -TCPADD -MAXHEAPPCT -STATISTICSINTERVAL -STATISTICSLIMIT -STATISTICSTYPE -STATISTICSENABLE -SQLPLAN -PORTMAPTOSECS -PORTBINDTOSECS";
+ public static final String DEFAULT_DCS_SERVER_USER_PROGRAM_COMMAND = "cd ${dcs.user.program.home};. ./sqenv.sh;mxosrvr -ZKHOST -RZ -ZKPNODE -CNGTO -ZKSTO -EADSCO -TCPADD -MAXHEAPPCT -STATISTICSINTERVAL -STATISTICSLIMIT -STATISTICSTYPE -STATISTICSENABLE -SQLPLAN -PORTMAPTOSECS -PORTBINDTOSECS -KEEPALIVEOPT";
--- End diff --
Got it.
---
[GitHub] trafodion pull request #1487: Trafodion keepalive support
Posted by hegdean <gi...@git.apache.org>.
Github user hegdean commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r175845635
--- Diff: dcs/src/main/java/org/trafodion/dcs/Constants.java ---
@@ -92,7 +92,7 @@
public static final String DCS_SERVER_USER_PROGRAM_COMMAND = "dcs.server.user.program.command";
/** Default value for DCS server user program command */
- public static final String DEFAULT_DCS_SERVER_USER_PROGRAM_COMMAND = "cd ${dcs.user.program.home};. ./sqenv.sh;mxosrvr -ZKHOST -RZ -ZKPNODE -CNGTO -ZKSTO -EADSCO -TCPADD -MAXHEAPPCT -STATISTICSINTERVAL -STATISTICSLIMIT -STATISTICSTYPE -STATISTICSENABLE -SQLPLAN -PORTMAPTOSECS -PORTBINDTOSECS";
+ public static final String DEFAULT_DCS_SERVER_USER_PROGRAM_COMMAND = "cd ${dcs.user.program.home};. ./sqenv.sh;mxosrvr -ZKHOST -RZ -ZKPNODE -CNGTO -ZKSTO -EADSCO -TCPADD -MAXHEAPPCT -STATISTICSINTERVAL -STATISTICSLIMIT -STATISTICSTYPE -STATISTICSENABLE -SQLPLAN -PORTMAPTOSECS -PORTBINDTOSECS -KEEPALIVEOPT";
--- End diff --
Instead of KEEPALIVEOPT change this to TCPKEEPALIVESTATUS, TCPKEEPALIVEIDLETIME, TCPKEEPALIVEINTERVAL, TCPKEEPALIVERETRYLIMIT
---
[GitHub] trafodion pull request #1487: Trafodion keepalive support
Posted by hegdean <gi...@git.apache.org>.
Github user hegdean commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r175844132
--- Diff: dcs/src/main/resources/dcs-default.xml ---
@@ -386,4 +386,12 @@
Timeout minutes between first and max times (6 default) DCS Server startup MXOSRVR.
</description>
</property>
+ <property>
+ <name>dcs.server.user.program.keepalive.opt</name>
+ <value>ENABLE=enable,IDLETIME=300,INTERTIME=5,RETRYCNT=3</value>
--- End diff --
Can we split this into multiple property/values and change the property name
dcs.server.user.program.tcp.keepalive.status
dcs.server.user.program.tcp.keepalive.idletime
dcs.server.user.program.tcp.keepalive.intervaltime
dcs.server.user.program.tcp.keepalive.retrycount
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by DaveBirdsall <gi...@git.apache.org>.
Github user DaveBirdsall commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1487#discussion_r176479622
--- Diff: core/conn/odbc/src/odbc/nsksrvr/SrvrMain.cpp ---
@@ -1427,7 +1437,59 @@ BOOL getInitParamSrvr(int argc, char *argv[], SRVR_INIT_PARAM_Def &initParam, ch
argEmpty = TRUE;
break;
}
- }
+ }else
+ if (strcmp(arg, "-TCPKEEPALIVESTATUS") == 0){
+ if (++count < argc && argv[count][0] != '-')
+ {
+ if (strlen(argv[count]) < sizeof(keepaliveStatus) - 1)
+ {
+ memset(keepaliveStatus, 0, sizeof(keepaliveStatus));
+ strncpy(keepaliveStatus, argv[count], sizeof(keepaliveStatus));
+ }
+ else
+ {
+ argWrong = TRUE;
+ }
+ }
+ else
+ {
+ argEmpty = TRUE;
+ break;
+ }
+ }else
+ if (strcmp(arg, "-TCPKEEPALIVEIDLETIME") == 0){
+ if (++count < argc )
+ {
+ keepaliveIdletime = atoi(argv[count]);
--- End diff --
If argv[count] isn't numeric, you'll get zero here. Or if it is a string like '123xyz456', you will get 123 here. Is this OK? Or would you rather have something more robust? Similar comments apply to the other parameters below.
---
[GitHub] trafodion pull request #1487: [TRAFODION-3003]Trafodion keepalive support
Posted by CoderSong2015 <gi...@git.apache.org>.
GitHub user CoderSong2015 reopened a pull request:
https://github.com/apache/trafodion/pull/1487
[TRAFODION-3003]Trafodion keepalive support
Keepalive could be configured by modifying file src/main/java/org/trafodion/dcs/Constants.java or src/main/resources/dcs-default.xml
Modify variable DCS_SERVER_PROGRAM_KEEPALIVE_OPT;
It's a string with parameter ENABLE IDLETIME INTERTIME RETRYCNT (Seconds)
ENABLE=enable/unenable/default ,IDLETIME= number ,INTERTIME=number,RETRYCNT=number
e.g:
ENABLE=enable,IDLETIME=300,INTERTIME=5,RETRYCNT=3
Default value is 1,3600,3,20(Only effective when value configured is set incorrectly)
The value will be read in when mxosrvr start. Mxosrvr will set the socket after getting a connection.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/CoderSong2015/Apache-Trafodion keepalive
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1487.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 #1487
----
commit 65bdc2f1365241b089c8568dc2a2aea52939916a
Author: Haolin.song <40...@...>
Date: 2018-03-20T15:31:31Z
[TRAFODION-3003]Trafodion keepalive support
Keepalive could be configured by modifying file src/main/java/org/trafodion/dcs/Constants.java
Modify variable DCS_SERVER_PROGRAM_TCP_KEEPALIVE_STATUS/IDLETIME/INTERVAL/RETRYCOUNT;
DCS_SERVER_PROGRAM_TCP_KEEPALIVE_STATUS has three value:enable,default,unenable;
Default value is enable,3600,3,20(Only effective when value configured is set incorrectly)
The value will be read in when mxosrvr start. Mxosrvr will set the socket after getting a connection.
----
---