Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/19523 )
Change subject: KUDU-1945 Auto-incrementing column feature flag ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/19523/2//COMMIT_MSG Commit Message: PS2: > Probably add the details on the behavior with newer client vs older server If I understand correctly, the RPC level verification falls back to rejecting a feature if the flag is not present on the server. (either because it is deliberately turned off, or it is not yet present - older server -) ServicePool::QueueInboundCall() -> MasterServiceImpl::SupportsFeature() Added a sentence to the commit message. http://gerrit.cloudera.org:8080/#/c/19523/2//COMMIT_MSG@9 PS2, Line 9: This patch adds the flag "--master_support_auto_incrementing_column" to : guard the auto-incrementing column feature > According to your explain at another thread. I think the commit messages ca Yes initially that would have been better. As of know, the plan is to get the missing patch merged. This would make the feature complete. I think the phrasing is okay this way. http://gerrit.cloudera.org:8080/#/c/19523/2//COMMIT_MSG@12 PS2, Line 12: to true in this patch. The verification happens on the RPC level. If the > nit: Could you mention the default value of the new flag in commit message? Done http://gerrit.cloudera.org:8080/#/c/19523/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/19523/2/src/kudu/master/master_service.cc@121 PS2, Line 121: master_support_auto_incrementing_column > 'master_support_auto_incrementing_column' is added at kudu-master, so 'sup Yes exactly! Thanks for pointing this out. Added the tags: experimental, unsafe. Regarding the naming of the flag: maybe I'm missing something, but all of the other flags are prefixed with 'master', thats why I named the flag in a similar fashion. http://gerrit.cloudera.org:8080/#/c/19523/2/src/kudu/master/master_service.cc@124 PS2, Line 124: TAG_FLAG(master_support_auto_incrementing_column, unsafe); > This flag is only for test, right? So we can mark it as 'hidden', maybe 'ru At this stage yes. I marked it as 'unsafe', according to the docs, this tag already hides it from user-facing docs. Regarding the 'runtime' tag, in flag_tags.h it has a big NOTE tag, I did not consider that. So it might not work with the current rev. Let me know if you think 'runtime' would make sense for this flag, and I will look into it. -- To view, visit http://gerrit.cloudera.org:8080/19523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39c3dde3705c25c36d3ad787c0db6ed03f6c2cfd Gerrit-Change-Number: 19523 Gerrit-PatchSet: 3 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Sat, 04 Mar 2023 14:28:39 +0000 Gerrit-HasComments: Yes
