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/27 15:00:44 UTC
[GitHub] trafodion pull request #1499: [TRAFODION-3003]Trafodion keepalive support
GitHub user CoderSong2015 opened a pull request:
https://github.com/apache/trafodion/pull/1499
[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,300,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/1499.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 #1499
----
commit 1b19b963c8284f974a76e8eed8d7cd433a536024
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,300,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 #1499: [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/1499#discussion_r177620171
--- Diff: dcs/src/main/java/org/trafodion/dcs/server/ServerManager.java ---
@@ -83,6 +83,10 @@
private int maxRestartAttempts;
private int retryIntervalMillis;
private String nid = null;
+ private static String mxosrvrKeepaliveStatus;
--- End diff --
OK.
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]Trafodion keepalive support
Posted by xiaozhongwang <gi...@git.apache.org>.
Github user xiaozhongwang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1499#discussion_r177487560
--- Diff: core/conn/odbc/src/odbc/nsksrvr/Interface/linux/Listener_srvr_ps.cpp ---
@@ -263,7 +263,11 @@ void* CNSKListenerSrvr::OpenTCPIPSession()
//LCOV_EXCL_STOP
}
-
+ TCP_SetKeepalive(nSocketFnum,
--- End diff --
it's better to use single name style in one name.
---
[GitHub] trafodion pull request #1499: [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/1499#discussion_r177961670
--- Diff: core/conn/odbc/src/odbc/Common/Global.h ---
@@ -139,10 +139,16 @@ class ODBCMXTraceMsg;
#define DEFAULT_REFRESH_RATE_SECS 60
#define DEFAULT_SRVR_IDLE_TIMEOUT 0
#define DEFAULT_CONN_IDLE_TIMEOUT 0
+#define DEFAULT_KEEPALIVE 1 //OPEN KEEPALIVE
+#define DEFAULT_KEEPALIVE_TIMESEC 3600
+#define DEFAULT_KEEPALIVE_COUNT 3
--- End diff --
I think 3 times and 5 times are both enough here. Can your tell me why it is better to retry more times?
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]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/1499#discussion_r177529786
--- Diff: dcs/src/main/resources/dcs-default.xml ---
@@ -386,4 +386,33 @@
Timeout minutes between first and max times (6 default) DCS Server startup MXOSRVR.
</description>
</property>
+ <property>
+ <name>dcs.server.user.program.tcp.keepalive.status</name>
+ <value>enable</value>
+ <description>
+ Used in mxosrvr keepalive , parameter is ENABLE IDLETIME INTERTIME RETRYCNT.
--- End diff --
Description should be specific to the property name/value.. specify the default for the property. Look at how the description for statistics enable is done. This applies to the other tcp keepalive properties as well
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]Trafodion keepalive support
Posted by xiaozhongwang <gi...@git.apache.org>.
Github user xiaozhongwang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1499#discussion_r177483293
--- 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 --
Why use clientKeepaliveStatus instead of keepalive option?
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]Trafodion keepalive support
Posted by xiaozhongwang <gi...@git.apache.org>.
Github user xiaozhongwang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1499#discussion_r177489126
--- Diff: dcs/src/main/java/org/trafodion/dcs/Constants.java ---
@@ -112,6 +112,29 @@
/** Default value for DCS server user program exit after disconnect */
public static final int DEFAULT_DCS_SERVER_USER_PROGRAM_EXIT_AFTER_DISCONNECT = 0;
+ /** Configuration key for DCS server program mxosrvr keepalive STATUS*/
+ public static final String DEFAULT_DCS_SERVER_PROGRAM_TCP_KEEPALIVE_STATUS= "dcs.server.user.program.tcp.keepalive.status";
+
+ /** Default value for DCS server program mxosrvr keepalive STATUS*/
+ public static final String DCS_SERVER_PROGRAM_KEEPALIVE_STATUS = "enable";
+
+ /** Configuration key for DCS server program mxosrvr keepalive IDLETIME*/
+ public static final String DEFAULT_DCS_SERVER_PROGRAM_TCP_KEEPALIVE_IDLETIME = "dcs.server.user.program.tcp.keepalive.idletime";
+
+ /** Default value for DCS server program mxosrvr keepalive IDLETIME*/
+ public static final int DCS_SERVER_PROGRAM_KEEPALIVE_IDLETIME = 300;
--- End diff --
Why client side use different value with server?
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]Trafodion keepalive support
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1499
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]Trafodion keepalive support
Posted by xiaozhongwang <gi...@git.apache.org>.
Github user xiaozhongwang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1499#discussion_r177485881
--- Diff: core/conn/odbc/src/odbc/Common/Global.h ---
@@ -139,10 +139,16 @@ class ODBCMXTraceMsg;
#define DEFAULT_REFRESH_RATE_SECS 60
#define DEFAULT_SRVR_IDLE_TIMEOUT 0
#define DEFAULT_CONN_IDLE_TIMEOUT 0
+#define DEFAULT_KEEPALIVE 1 //OPEN KEEPALIVE
+#define DEFAULT_KEEPALIVE_TIMESEC 3600
+#define DEFAULT_KEEPALIVE_COUNT 3
--- End diff --
where the default value come from?
I think it's better to retry more times。
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]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/1499#discussion_r177530157
--- Diff: dcs/src/main/resources/dcs-default.xml ---
@@ -386,4 +386,33 @@
Timeout minutes between first and max times (6 default) DCS Server startup MXOSRVR.
</description>
</property>
+ <property>
+ <name>dcs.server.user.program.tcp.keepalive.status</name>
+ <value>enable</value>
+ <description>
+ Used in mxosrvr keepalive , parameter is ENABLE IDLETIME INTERTIME RETRYCNT.
+ </description>
+ </property>
+ <property>
+ <name>dcs.server.user.program.tcp.keepalive.idletime</name>
+ <value>300</value>
+ <description>
+ Used in mxosrvr keepalive , parameter is ENABLE IDLETIME INTERTIME RETRYCNT.
+ </description>
+ </property>
+ <property>
+ <name>dcs.server.user.program.tcp.keepalive.intervaltime</name>
+ <value>5</value>
+ <description>
+ Used in mxosrvr keepalive , parameter is ENABLE IDLETIME INTERTIME RETRYCNT.
--- End diff --
description should be specific to the property. Also would be good to indicate the unit of time mins, secs or msec and so on
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]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/1499#discussion_r177527743
--- Diff: core/conn/odbc/src/odbc/nsksrvr/Interface/Listener_srvr.cpp ---
@@ -81,4 +81,49 @@ 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")){
--- End diff --
Can the status be boolean (TRUE or FALSE) and default value can be set FALSE
---
[GitHub] trafodion pull request #1499: [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/1499#discussion_r177963928
--- Diff: core/conn/odbc/src/odbc/nsksrvr/Interface/linux/Listener_srvr_ps.cpp ---
@@ -263,7 +263,11 @@ void* CNSKListenerSrvr::OpenTCPIPSession()
//LCOV_EXCL_STOP
}
-
+ TCP_SetKeepalive(nSocketFnum,
--- End diff --
I think you are right, but I have seen the same name style in mxosrvr a lot of times, like in SrvrConnect.cpp and odbcas_drvr.cpp. So I think they are all allowed here.
---
[GitHub] trafodion pull request #1499: [TRAFODION-3003]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/1499#discussion_r177531067
--- Diff: dcs/src/main/java/org/trafodion/dcs/server/ServerManager.java ---
@@ -83,6 +83,10 @@
private int maxRestartAttempts;
private int retryIntervalMillis;
private String nid = null;
+ private static String mxosrvrKeepaliveStatus;
--- End diff --
can u change mxosrvr to userProg just like the other properties already defined
---