Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13796 )
Change subject: KUDU-2722 (table level): Support new 'write_enabled' table extra config ...................................................................... Patch Set 2: (2 comments) Yingchun has also been adding another extra config (https://gerrit.cloudera.org/c/13659/). In his review I asked whether we should update these unit tests every time a new extra config is added. I'm not sure it's necessary; could you take a look at that review comment in his review and add your thoughts? > > I'm curious about how to implement the read-only range partition? > > For the read-only range partitions, it seems that the tablet-based > > configuration might be better, not a table. :) > > Hello Yao Xu, talking about this with Grant before. Yeah, table level config > is not appropriate. An RPC would be used to set the tablet state to READ_ONLY > (https://github.com/apache/kudu/blob/master/src/kudu/tablet/metadata.proto#L58). > What do you think? @Adar Hmm. Ideally all tablet configuration would be mediated by the master, so that clients/tools need only connect to masters, not to individual tservers. Moreover, the configuration should be persisted, otherwise it'd need to be reapplied whenever a tserver reboots. What's tricky here is that we try hard to avoid exposing tablets to users; they normally think about tables instead. And for this feature we'd want to refer to tablets by partition key range (or by start partition key) rather than by tablet ID. Maybe solicit input from Grant, who filed the ticket originally? http://gerrit.cloudera.org:8080/#/c/13796/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/13796/2/src/kudu/tserver/tablet_service.cc@1084 PS2, Line 1084: boost::optional<TableExtraConfigPB> extra_config = tablet->metadata()->extra_config(); This makes a copy, so consider storing a cref: const auto& extra_config = tablet->metadata()->extra_config(). http://gerrit.cloudera.org:8080/#/c/13796/2/src/kudu/util/status.h File src/kudu/util/status.h: http://gerrit.cloudera.org:8080/#/c/13796/2/src/kudu/util/status.h@450 PS2, Line 450: kNotEnabled = 19, We're very very hesitant to introduce new Status codes. In this case, perhaps IllegalState would be OK? -- To view, visit http://gerrit.cloudera.org:8080/13796 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd3768eda36d9574be9c41e7cd3dd81cc5ae8f3a Gerrit-Change-Number: 13796 Gerrit-PatchSet: 2 Gerrit-Owner: XiaokaiWang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: XiaokaiWang <[email protected]> Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Comment-Date: Wed, 10 Jul 2019 15:28:05 +0000 Gerrit-HasComments: Yes
