Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19203 )
Change subject: [KUDU-3419][TabletManager] Fix the stuck of tablet server when starting ...................................................................... Patch Set 4: (9 comments) Thank you for the fix! http://gerrit.cloudera.org:8080/#/c/19203/4//COMMIT_MSG Commit Message: PS4: Could you reformat the commit message a bit? Please follow the guide at https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines that can be found at the https://kudu.apache.org/docs/contributing.html#_submitting_patches page. Thanks! http://gerrit.cloudera.org:8080/#/c/19203/4//COMMIT_MSG@7 PS4, Line 7: [KUDU-3419][TabletManager] nit: use either 'KUDU-3419: ...' or '[TabletManager] ...', but not both http://gerrit.cloudera.org:8080/#/c/19203/4//COMMIT_MSG@12 PS4, Line 12: it's its http://gerrit.cloudera.org:8080/#/c/19203/4//COMMIT_MSG@14 PS4, Line 14: There are not change to finish this task nit: There was no code to shutdown the task in that case. http://gerrit.cloudera.org:8080/#/c/19203/4//COMMIT_MSG@14 PS4, Line 14: Therefore tablet server will get stuck In addition, does it make sense updating the code of the TabletServer class to account for the transitioning from TabletServerState::kStopped to TabletServerState::kInitialized? Say, introduce TabletServerState::kInitializing and set state_ to kInitializing upon entry to the Init() method. With that, it's also necessary to update the TabletServer::Shutdown() method to take into consideration the TabletServerState::kInitializing state as well. That's to have more control over the shutdown sequence in TabletServer. http://gerrit.cloudera.org:8080/#/c/19203/4//COMMIT_MSG@15 PS4, Line 15: See the detail at #KUDU-3419 nit: See KUDU-3419 for details. http://gerrit.cloudera.org:8080/#/c/19203/4/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/19203/4/src/kudu/tserver/ts_tablet_manager.h@196 PS4, Line 196: void GetLocalTabletReplicas( : std::vector<scoped_refptr<tablet::TabletReplica> >* replicas) const; Make this method private and call it in the implementation of the GetTabletReplicas(). In addition, it's all about local replicas anyways -- information on remote replicas isn't maintained by TSTabletManager, so consider renaming the method, maybe? GetTabletReplicasImpl() might be a good alternative in this context. http://gerrit.cloudera.org:8080/#/c/19203/4/src/kudu/tserver/ts_tablet_manager.h@200 PS4, Line 200: nit: remove -- contemporary C++ compilers don't require this extra space http://gerrit.cloudera.org:8080/#/c/19203/4/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/19203/4/src/kudu/tserver/ts_tablet_manager.cc@553 PS4, Line 553: LOG_AND_RETURN(WARNING, first_error); I guess this should have become LOG_AND_RETURN(ERROR, first_error); -- To view, visit http://gerrit.cloudera.org:8080/19203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c9a4f4158fcb0a36499345e00ee72c65f5fefe0 Gerrit-Change-Number: 19203 Gerrit-PatchSet: 4 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sun, 06 Nov 2022 16:34:04 +0000 Gerrit-HasComments: Yes
