Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16698 )
Change subject: KUDU-1563. Add a feature flag for IGNORE operations ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/16698/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/16698/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@2662 PS3, Line 2662: // The server returns an RpcRemoteException when : // the required feature is not supported. : if (resp instanceof RpcRemoteException) { : return false; : } Does it make sense to be more specific here? As I understand, the server should set error code to ERROR_INVALID_REQUEST and populate the list of unsupported features. At least check that the code is ERROR_INVALID_REQUEST and that an instance of the RpcRemoteExpection contains at least one feature flag (and that would be the requested one). Otherwise, it might be ServiceUnavailable be interpreted as a not-support-a-feature response, no? http://gerrit.cloudera.org:8080/#/c/16698/3/java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java: http://gerrit.cloudera.org:8080/#/c/16698/3/java/kudu-client/src/main/java/org/apache/kudu/client/PingRequest.java@43 PS3, Line 43: makeMasterPingRequest I don't have good understanding of exact workflow here, but would it be more practical to supply the required list of features into the ConnectToMaster RPC? That way the client would know what cluster it has connected to from the very beginning. http://gerrit.cloudera.org:8080/#/c/16698/3/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/16698/3/src/kudu/master/master_service.cc@106 PS3, Line 106: TAG_FLAG nit: does it make sense to add the 'runtime' tag as well? -- To view, visit http://gerrit.cloudera.org:8080/16698 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I329bd8bde73d247240ae597b677e2cc20a92343a Gerrit-Change-Number: 16698 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 07 Nov 2020 03:07:30 +0000 Gerrit-HasComments: Yes
