Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18866 )
Change subject: [master] KUDU-3322 / KUDU-3319 HMS event logging ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/18866/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18866/2//COMMIT_MSG@15 PS2, Line 15: Was hesitant on crashing the : master and chose to log an ERROR as in the case an out of order event : id received from HMS might lead to unnecessary crash of the Kudu : master > What parts of the system are still functional after this happening? I gues Yes DDL operations especially drop/alter tables don't work. Create table works. I think we can have a followup change list to introduce a kudu hms unsafe eventid <new event id. Default: 0> to recover from this situation. http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/hms/hms_catalog.h@147 PS2, Line 147: Status GetCurrentNotificationEventId(int64_t* event_id) WARN_UNUSED_RESULT; > nit: could this method be constant? No, because the subsequent calls made in the body of the method cannot be const. http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/hms/mini_hms.h@75 PS2, Line 75: // Delete the HMS database directory. > nit: follow the style of the comments in the rest of this file -- add a per Done http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/hms/mini_hms.cc@188 PS2, Line 188: return Env::Default()->DeleteRecursively(JoinPathSegments(data_root_, metadb_subdir_)); : } > nit: could be more concise Done http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/integration-tests/master_hms-itest.cc@88 PS2, Line 88: StartClusterWithOpts(std::move(opts)); > Maybe, create its own class to change this behavior just for the sake of a Done http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/integration-tests/master_hms-itest.cc@240 PS2, Line 240: se("db")); > Why is it needed? Why not to go with just 'default.a' -- that would be one I found that if there is only one HMS event the event ID published by HMS is not 0. Not sure of the behavior of the HMS when there is only 1 event present. Hence created more than 1 event. Will comment that here. http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/integration-tests/master_hms-itest.cc@263 PS2, Line 263: TR_CONTAINS(s.ToString(), kInvalidHiveTableError); > Why not to search for the whole error message as at line 264? Done http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/integration-tests/master_hms-itest.cc@271 PS2, Line 271: > nit: the indent seems to be messed up Done http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/master/catalog_manager.cc@1454 PS2, Line 1454: > nit: why is this change? I guess the 'if ...' line should have been shifte The return is in the 'if' block, right? I'll shift the 'if' line and the corresponding ones as well. http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/master/catalog_manager.cc@1457 PS2, Line 1457: } > Maybe, move this into Done http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/master/hms_notification_log_listener.cc File src/kudu/master/hms_notification_log_listener.cc: http://gerrit.cloudera.org:8080/#/c/18866/2/src/kudu/master/hms_notification_log_listener.cc@295 PS2, Line 295: LOG(ERROR) << Substitute( "Found the last seen event ID in the local Kudu master($0) " : "to be greater than the latest event ID in HMS($1)" : ". Was there any backup or restore done on HMS > nit: maybe, use Substitute() to make this more readable? Done -- To view, visit http://gerrit.cloudera.org:8080/18866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I242c9cedf170ed621867b905f1cb1875347d5887 Gerrit-Change-Number: 18866 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Sep 2022 17:51:20 +0000 Gerrit-HasComments: Yes
