Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12468 )
Change subject: KUDU-2514 Part 1: Support extra config for table. ...................................................................... Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java File java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java: http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@39 PS8, Line 39: private final static Set<String> supportedKeys = new HashSet<>(Arrays.asList( > > I don't think the client should be in the business of deciding I don't see why the RPC would be any larger or smaller. The configuration options are statically defined in the master's persistent state, so even though we'd convert that into a simpler map on the wire to the client, there's still a clear upper bound on the size of the config map. Sure, a client could stuff the map with thousands of entries, and eventually the RPC might be too big and get rejected, but that's misbehavior on the part of the client, as we would never have thousands of distinct per-table configuration options. BTW, you don't need to define your own key-value pair message; protobuf supports maps. Check out https://developers.google.com/protocol-buffers/docs/proto3#maps. http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@40 PS8, Line 40: TABLE_HISTORY_MAX_AGE_SEC > > This set should be empty in this patch, right? Yeah I figured as much. Don't get too worried about it; if it's much more difficult to test without the inclusion of the one config, then by all means keep it in this patch. -- To view, visit http://gerrit.cloudera.org:8080/12468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0514507dca95602a97e954d1db464b907e073aae Gerrit-Change-Number: 12468 Gerrit-PatchSet: 8 Gerrit-Owner: Yao Xu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Comment-Date: Fri, 17 May 2019 17:43:52 +0000 Gerrit-HasComments: Yes
