Yao Xu 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 20: (4 comments) http://gerrit.cloudera.org:8080/#/c/12468/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java: http://gerrit.cloudera.org:8080/#/c/12468/19/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@32 PS19, Line 32: import java.util.Map; > nit: could you stick this before java.util.List so it's alphabetical? Done http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc@656 PS19, Line 656: kudu. > Why "table" and why "kudu"? The config maps are *Table*ExtraConfigPB. > Do we expect these configs to ever _not_ be table configs or Kudu > configs? > > Since these will be stored on disk, even though it's a small amount > of space, could we do without the "kudu.table"? Or does it provide > value via uniqueness of keys in some way? Emm, these prefixes are added to make it easier for users to understand what the configuration means when using SQL to modify the configuration. We don't store these strings on disk. The actual storage is the serialized of TableExtraConfig. http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc@668 PS19, Line 668: t string& na > nit: It would be more straightforward if you pulled out config.first and co Done http://gerrit.cloudera.org:8080/#/c/12468/19/src/kudu/common/wire_protocol.cc@672 PS19, Line 672: > nit: Should this be "kudu.table.history_max_age_sec"? Maybe use Substitute( Done -- 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: 20 Gerrit-Owner: Yao Xu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[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: Sat, 08 Jun 2019 04:36:41 +0000 Gerrit-HasComments: Yes
