Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14920 )
Change subject: KUDU-2975: Spread WAL across multiple directories ...................................................................... Patch Set 7: (12 comments) I rebased to get this at least up to master. Yang Song may not be able to finish this -- I'll try picking it up and splitting it up into smaller chunks. http://gerrit.cloudera.org:8080/#/c/14920/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14920/6//COMMIT_MSG@26 PS6, Line 26: AL direc > nit: tablet's Done http://gerrit.cloudera.org:8080/#/c/14920/6//COMMIT_MSG@26 PS6, Line 26: AL directories. : If found, we write the tablet's WAL into WalDirManager. We'll first : register the WAL director > nit: you can probably omit this and instead describe what we do: Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.h File src/kudu/consensus/log.h: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.h@322 PS6, Line 322: > nit: could you also describe 'wal_path' and how it can be empty? Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/consensus/log.cc@1208 PS6, Line 1208: } else { > Remove? Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager.cc@682 PS6, Line 682: data > nit: wal Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/fs_manager.cc@795 PS6, Line 795: // Temporary files in the Block Manager directories are cleaned during : // Block Manager startup. > This should be true for the WAL manager too, right? Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs.h File src/kudu/fs/wal_dirs.h: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/fs/wal_dirs.h@131 PS6, Line 131: // Deletes the WAL directory for the specified tablet. Maps from tablet_id to > nit: can you specify that it only unregisters the directory in memory? Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tablet/tablet_metadata.cc@467 PS6, Line 467: A_READ > nit: "WAL directory" so it's consistent with other messages Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_fs.cc@349 PS6, Line 349: s1.ToString(); > Maybe print both? Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tools/tool_action_local_replica.cc@523 PS6, Line 523: } : > nit: maybe reword a bit: Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/ts_tablet_manager.cc@1465 PS6, Line 1465: > nit: could you add a comment why we're doing this before deleting the metad Done http://gerrit.cloudera.org:8080/#/c/14920/6/src/kudu/tserver/ts_tablet_manager.cc@1484 PS6, Line 1484: string wal_dir; : string wal_recovery_dir; : WARN_NOT_OK(meta > nit: maybe condition on !wal_dir.empty() and !wal_recovery_dir.empty() resp Done -- To view, visit http://gerrit.cloudera.org:8080/14920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3490b2bbc0544e7c8a5f987fb83855ff155ac2c Gerrit-Change-Number: 14920 Gerrit-PatchSet: 7 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Fri, 13 Mar 2020 00:38:22 +0000 Gerrit-HasComments: Yes
