You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2020/10/22 11:26:42 UTC
[GitHub] [iotdb] Alima777 opened a new pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Alima777 opened a new pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 commented on pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Posted by GitBox <gi...@apache.org>.
Alima777 commented on pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846#issuecomment-717796018
> Consider add some setting timezone tests in `SessionTest` or `SessionPoolTest`?
Added~
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 commented on a change in pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846#discussion_r512576025
##########
File path: thrift/src/main/thrift/rpc.thrift
##########
@@ -79,9 +79,10 @@ struct TSOpenSessionResp {
// Open a session (connection) on the server against which operations may be executed.
struct TSOpenSessionReq {
1: required TSProtocolVersion client_protocol = TSProtocolVersion.IOTDB_SERVICE_PROTOCOL_V3
- 2: optional string username
- 3: optional string password
- 4: optional map<string, string> configuration
+ 2: required string zoneId
+ 3: optional string username
+ 4: optional string password
+ 5: optional map<string, string> configuration
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] HTHou merged pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Posted by GitBox <gi...@apache.org>.
HTHou merged pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Alima777 commented on a change in pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Posted by GitBox <gi...@apache.org>.
Alima777 commented on a change in pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846#discussion_r512578768
##########
File path: session/src/test/java/org/apache/iotdb/session/pool/SessionPoolTest.java
##########
@@ -163,7 +163,7 @@ private void correctQuery(SessionPool pool) {
@Test
public void tryIfTheServerIsRestart() {
- SessionPool pool = new SessionPool("127.0.0.1", 6667, "root", "root", 3, 1, 6000, false);
+ SessionPool pool = new SessionPool("127.0.0.1", 6667, "root", "root", 3, 1, 6000, false, null);
Review comment:
Not Exactly. We can use a constructor which doesn't need a timezone parameter instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] liutaohua commented on a change in pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Posted by GitBox <gi...@apache.org>.
liutaohua commented on a change in pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846#discussion_r511895980
##########
File path: session/src/main/java/org/apache/iotdb/session/Session.java
##########
@@ -916,23 +922,14 @@ public boolean checkTimeseriesExists(String path)
return result;
}
- private synchronized String getTimeZone()
- throws StatementExecutionException, IoTDBConnectionException {
+ private synchronized String getTimeZone() {
if (zoneId != null) {
return zoneId.toString();
}
-
- TSGetTimeZoneResp resp;
- try {
- resp = client.getTimeZone(sessionId);
- } catch (TException e) {
- throw new IoTDBConnectionException(e);
- }
- RpcUtils.verifySuccess(resp.getStatus());
- return resp.getTimeZone();
+ return ZoneId.systemDefault().getId();
Review comment:
There is something wrong with the logic.
It should be the default value that the server stores when the client is not registered with the server.
##########
File path: session/src/main/java/org/apache/iotdb/session/Session.java
##########
@@ -916,23 +922,14 @@ public boolean checkTimeseriesExists(String path)
return result;
}
- private synchronized String getTimeZone()
- throws StatementExecutionException, IoTDBConnectionException {
+ private synchronized String getTimeZone() {
if (zoneId != null) {
return zoneId.toString();
}
-
- TSGetTimeZoneResp resp;
- try {
- resp = client.getTimeZone(sessionId);
- } catch (TException e) {
- throw new IoTDBConnectionException(e);
- }
- RpcUtils.verifySuccess(resp.getStatus());
- return resp.getTimeZone();
+ return ZoneId.systemDefault().getId();
}
- private synchronized void setTimeZone(String zoneId)
+ public synchronized void setTimeZone(String zoneId)
throws StatementExecutionException, IoTDBConnectionException {
TSSetTimeZoneReq req = new TSSetTimeZoneReq(sessionId, zoneId);
TSStatus resp;
Review comment:
There may be a bug here, because the value stored by the server does not necessarily change when an error is encountered, but the local cached zoneId does
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] HTHou commented on a change in pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Posted by GitBox <gi...@apache.org>.
HTHou commented on a change in pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846#discussion_r512434392
##########
File path: session/src/test/java/org/apache/iotdb/session/pool/SessionPoolTest.java
##########
@@ -163,7 +163,7 @@ private void correctQuery(SessionPool pool) {
@Test
public void tryIfTheServerIsRestart() {
- SessionPool pool = new SessionPool("127.0.0.1", 6667, "root", "root", 3, 1, 6000, false);
+ SessionPool pool = new SessionPool("127.0.0.1", 6667, "root", "root", 3, 1, 6000, false, null);
Review comment:
So we have to fill a `null` to the constructor of SessionPool if we don't want to set timezone, right?
I think we'd better don't change these tests and add an extra test of setting timezone.
##########
File path: thrift/src/main/thrift/rpc.thrift
##########
@@ -79,9 +79,10 @@ struct TSOpenSessionResp {
// Open a session (connection) on the server against which operations may be executed.
struct TSOpenSessionReq {
1: required TSProtocolVersion client_protocol = TSProtocolVersion.IOTDB_SERVICE_PROTOCOL_V3
- 2: optional string username
- 3: optional string password
- 4: optional map<string, string> configuration
+ 2: required string zoneId
+ 3: optional string username
+ 4: optional string password
+ 5: optional map<string, string> configuration
Review comment:
Please update `thrift/rpc-changelist.md`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] HTHou commented on pull request #1846: [IOTDB-872] Use system timezone in CLI (Session)
Posted by GitBox <gi...@apache.org>.
HTHou commented on pull request #1846:
URL: https://github.com/apache/iotdb/pull/1846#issuecomment-717660739
Consider add some setting timezone tests in `SessionTest` or `SessionPoolTest`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org