Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master ...................................................................... Patch Set 2: (9 comments) Did a high-level first pass so far. Didn't dig too deeply into the tests, but overall the approach seems sound. http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc@2470 PS2, Line 2470: const auto& consensus = cmeta_->ActiveConfig(); nit: maybe call this 'raft_config' or something to reflect what its type is. http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/consensus/raft_consensus.cc@2474 PS2, Line 2474: break; : } : } Would it make sense to make this function return a Status and return NotFound() if the local UUID is not in the Raft config? http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.h@781 PS2, Line 781: must be initialized before calling this method. nit: are there DCHECK()s we can add to enforce this? http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/catalog_manager.cc@5564 PS2, Line 5564: if (comp_status.ok()) { If this is not OK, how should we expect users to catch such errors? Is there any way we can make error-handling more straightforward, e.g. passing a callback from the MasterServiceImpl to call upon success/failure to return to the client? http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/dynamic_multi_master-test.cc@148 PS2, Line 148: new_master_opts.exe = cluster_->GetBinaryPath("kudu-master"); : new_master_opts.messenger = cluster_->messenger(); : new_master_opts.block_manager_type = cluster_->block_manager_type(); : new_master_opts.wal_dir = cluster_->GetWalPath(new_master_id); : new_master_opts.data_dirs = cluster_->GetDataPaths(new_master_id); : new_master_opts.log_dir = cluster_->GetLogPath(new_master_id); : new_master_opts.rpc_bind_address = reserved_hp_; : new_master_opts.start_process_timeout = cluster_->start_process_timeout(); Could we just copy some of the opts from an existing daemon? Seems then we'd only need to set wal_dir, data_dir, log_dir, rpc_bind_address, and extra_flags http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master.h@89 PS2, Line 89: MasterOptions& nit: consider returning a pointer instead for mutable accessors. That's the status quo for auto-generated protobuf at least. http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_options.h File src/kudu/master/master_options.h: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_options.h@57 PS2, Line 57: // Locks are not copyable, so using a static lock instead. MasterOptions is used during init : // time, so apart from tests there aren't usually multiple instances of MasterOptions. A static lock is incredibly surprising for things that aren't designed to be singletons. It seems wrong to share static mutable state across multiple instances of this class. If we need to copy a class, why not define a copy constructor? Also a simple_spinlock seems like it would be sufficient. http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@243 PS2, Line 243: if (!FLAGS_master_support_change_config) { : rpc->RespondFailure(Status::NotSupported("Adding master is not supported")); : return; : } Just FYI this can also be done via MasterServiceImpl::SupportsFeature() http://gerrit.cloudera.org:8080/#/c/16321/2/src/kudu/master/master_service.cc@255 PS2, Line 255: INFO nit: users should probably be concerned about this, so this should probably be a warning or error. -- To view, visit http://gerrit.cloudera.org:8080/16321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c Gerrit-Change-Number: 16321 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 10 Sep 2020 21:31:35 +0000 Gerrit-HasComments: Yes
