Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22529 )
Change subject: KUDU-3639 Add table operation functions follow-up ...................................................................... Patch Set 3: (5 comments) Thank you for putting together this follow-up patch! http://gerrit.cloudera.org:8080/#/c/22529/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/22529/3/src/kudu/master/catalog_manager.cc@442 PS3, Line 442: log_listener_catch_up_deadline_ms nit: maybe, name this hive_metastore_notification_log_listener_catch_up_deadline_ms to match the rest of the HMS-related flags (e.g., 'hive_metastore_notification_log_poll_period_seconds') and move the definition of the flag into hms_notification_log_listener.cc? http://gerrit.cloudera.org:8080/#/c/22529/3/src/kudu/master/catalog_manager.cc@443 PS3, Line 443: the log listener nit: the HMS notification log listener http://gerrit.cloudera.org:8080/#/c/22529/3/src/kudu/master/catalog_manager.cc@445 PS3, Line 445: TAG_FLAG(log_listener_catch_up_deadline_ms, advanced); Consider adding 'experimental' tag for this flag: at this point, it's not yet clear whether this flag is going to stay as-is when the work on the master's REST API is finished, right? Also, add the 'runtime' tag since the behavior is affected without restart by just updating the flag's value in runtime. http://gerrit.cloudera.org:8080/#/c/22529/3/src/kudu/master/catalog_manager.cc@6854 PS3, Line 6854: ( nit: parentheses aren't necessary here http://gerrit.cloudera.org:8080/#/c/22529/3/src/kudu/master/catalog_manager.cc@6855 PS3, Line 6855: ? rpc->GetClientDeadline() : : MonoTime::Now() + MonoDelta::FromMilliseconds(FLAGS_log_listener_catch_up_deadline_ms); : Status s = hms_notification_log_listener_->WaitForCatchUp(deadline); Instead, consider updating the signagure and implementation of HmsNotificationLogListenerTask::WaitForCatchUp() to make the 'deadline' parameter optional, and using the deadline based on the newly introduced flag if no deadline is specified by a caller. -- To view, visit http://gerrit.cloudera.org:8080/22529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39ed6526fc66113667c21051972da191d4ed209b Gerrit-Change-Number: 22529 Gerrit-PatchSet: 3 Gerrit-Owner: Gabriella Lotz <lotzgabrie...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Gabriella Lotz <lotzgabrie...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Zoltan Chovan <zcho...@cloudera.com> Gerrit-Comment-Date: Tue, 25 Feb 2025 02:24:24 +0000 Gerrit-HasComments: Yes
