This is an automated email from the ASF dual-hosted git repository. granthenke pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 4f82a46da97d9987139aee26614e904118bce4a9 Author: Grant Henke <[email protected]> AuthorDate: Sun Feb 17 14:48:15 2019 -0600 [tools] Support running the master and tablet server via the kudu binary Adds commands to the kudu tools to run the master and tablet server: - `kudu master run ...` - `kudu tserver run ...` This means we can ship a single binary in the kudu docker image or potentially the kudu-binary jar reducing the size by approximately 66%. Though follow up changes may be needed in the kudu-binary case. The behavior is the same as running the kudu-master and kudu-tserver binaries. The logging initialization was updated to ensure that the logging filenames are the same as when running via the dedicated binaries. A few logging fixes are included to ensure this capability is possible. This patch also contains a few related fixes for default flags ensuring that they can be overwritten when necessary. These changes also ensure flags are not incorrectly reported as non-default. Change-Id: I3717cbac930b3506a76f7a51388c64afbcbb480e Reviewed-on: http://gerrit.cloudera.org:8080/12517 Tested-by: Grant Henke <[email protected]> Reviewed-by: Adar Dembo <[email protected]> Reviewed-by: Andrew Wong <[email protected]> Reviewed-by: Alexey Serbin <[email protected]> --- src/kudu/master/CMakeLists.txt | 1 + src/kudu/master/master-test.cc | 2 + src/kudu/master/master_main.cc | 73 +----------------- .../master/{master_main.cc => master_runner.cc} | 89 ++++++++-------------- src/kudu/master/master_runner.h | 31 ++++++++ src/kudu/server/diagnostics_log.cc | 4 +- src/kudu/server/diagnostics_log.h | 3 +- src/kudu/server/server_base.cc | 4 +- src/kudu/tools/kudu-tool-test.cc | 12 +-- src/kudu/tools/tool_action.cc | 27 ++++++- src/kudu/tools/tool_action.h | 15 ++++ src/kudu/tools/tool_action_master.cc | 40 +++++++++- src/kudu/tools/tool_action_tserver.cc | 41 +++++++++- src/kudu/tools/tool_main.cc | 8 +- src/kudu/tserver/CMakeLists.txt | 1 + src/kudu/tserver/tablet_server.h | 1 + src/kudu/tserver/tablet_server_main.cc | 55 +------------ ...blet_server_main.cc => tablet_server_runner.cc} | 52 +++++-------- src/kudu/tserver/tablet_server_runner.h | 31 ++++++++ src/kudu/util/flag_tags-test.cc | 16 ++-- src/kudu/util/flags-test.cc | 9 +-- src/kudu/util/flags.cc | 28 ++++--- src/kudu/util/flags.h | 7 +- src/kudu/util/logging.cc | 10 ++- src/kudu/util/logging.h | 1 + src/kudu/util/minidump.cc | 12 ++- src/kudu/util/rolling_log-test.cc | 6 +- src/kudu/util/rolling_log.cc | 10 ++- src/kudu/util/rolling_log.h | 6 +- 29 files changed, 316 insertions(+), 279 deletions(-) diff --git a/src/kudu/master/CMakeLists.txt b/src/kudu/master/CMakeLists.txt index 39092fd..d4e27be 100644 --- a/src/kudu/master/CMakeLists.txt +++ b/src/kudu/master/CMakeLists.txt @@ -41,6 +41,7 @@ set(MASTER_SRCS master_cert_authority.cc master_options.cc master_path_handlers.cc + master_runner.cc master_service.cc mini_master.cc placement_policy.cc diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc index 331e502..dfb13cd 100644 --- a/src/kudu/master/master-test.cc +++ b/src/kudu/master/master-test.cc @@ -116,6 +116,7 @@ DECLARE_int32(master_inject_latency_on_tablet_lookups_ms); DECLARE_int64(live_row_count_for_testing); DECLARE_int64(on_disk_size_for_testing); DECLARE_string(location_mapping_cmd); +DECLARE_string(log_filename); namespace kudu { namespace master { @@ -913,6 +914,7 @@ TEST_F(MasterTest, TestDumpStacksOnRpcQueueOverflow) { // Use a new log directory so that the tserver and master don't share the // same one. This allows us to isolate the diagnostics log from the master. FLAGS_log_dir = JoinPathSegments(GetTestDataDirectory(), "master-logs"); + FLAGS_log_filename = "kudu-master"; Status s = env_->CreateDir(FLAGS_log_dir); ASSERT_TRUE(s.ok() || s.IsAlreadyPresent()) << s.ToString(); mini_master_->Shutdown(); diff --git a/src/kudu/master/master_main.cc b/src/kudu/master/master_main.cc index 7066925..613bdb8 100644 --- a/src/kudu/master/master_main.cc +++ b/src/kudu/master/master_main.cc @@ -16,95 +16,28 @@ // under the License. #include <iostream> -#include <string> -#include <gflags/gflags.h> #include <glog/logging.h> -#include "kudu/gutil/strings/substitute.h" -#include "kudu/master/master.h" -#include "kudu/util/flag_validators.h" +#include "kudu/master/master_runner.h" #include "kudu/util/flags.h" #include "kudu/util/init.h" #include "kudu/util/logging.h" -#include "kudu/util/monotime.h" #include "kudu/util/status.h" -#include "kudu/util/version_info.h" - -using kudu::master::Master; - -DECLARE_bool(evict_failed_followers); -DECLARE_int32(webserver_port); -DECLARE_string(rpc_bind_addresses); - -DECLARE_bool(hive_metastore_sasl_enabled); -DECLARE_string(keytab_file); namespace kudu { namespace master { -namespace { -// Validates that if the HMS is configured with SASL enabled, the server has a -// keytab available. This is located in master.cc because the HMS module (where -// -hive_metastore_sasl_enabled is defined) doesn't link to the server module -// (where --keytab_file is defined), and vice-versa. The master module is the -// first module which links to both. -// Note: this check only needs to be run on a server. E.g. tools that run with -// the HMS don't need to pass in a keytab. -bool ValidateHiveMetastoreSaslEnabled() { - if (FLAGS_hive_metastore_sasl_enabled && - FLAGS_keytab_file.empty()) { - LOG(ERROR) << "When the Hive Metastore has SASL enabled " - "(--hive_metastore_sasl_enabled), Kudu must be configured with " - "a keytab (--keytab_file)."; - return false; - } - return true; -} -GROUP_FLAG_VALIDATOR(hive_metastore_sasl_enabled, ValidateHiveMetastoreSaslEnabled); -} // anonymous namespace - static int MasterMain(int argc, char** argv) { RETURN_MAIN_NOT_OK(InitKudu(), "InitKudu() failed", 1); - - // Reset some default values before parsing gflags. - FLAGS_rpc_bind_addresses = strings::Substitute("0.0.0.0:$0", - Master::kDefaultPort); - FLAGS_webserver_port = Master::kDefaultWebPort; - - // A multi-node Master leader should not evict failed Master followers - // because there is no-one to assign replacement servers in order to maintain - // the desired replication factor. (It's not turtles all the way down!) - FLAGS_evict_failed_followers = false; - - // Setting the default value of the 'force_block_cache_capacity' flag to - // 'false' makes the corresponding group validator enforce proper settings - // for the memory limit and the cfile cache capacity. - CHECK_NE("", SetCommandLineOptionWithMode("force_block_cache_capacity", - "false", gflags::SET_FLAGS_DEFAULT)); - - GFlagsMap default_flags = GetFlagsMap(); - + SetMasterFlagDefaults(); ParseCommandLineFlags(&argc, &argv, true); if (argc != 1) { std::cerr << "usage: " << argv[0] << std::endl; return 2; } - std::string nondefault_flags = GetNonDefaultFlags(default_flags); InitGoogleLoggingSafe(argv[0]); - - LOG(INFO) << "Master server non-default flags:\n" - << nondefault_flags << '\n' - << "Master server version:\n" - << VersionInfo::GetAllVersionInfo(); - - Master server({}); - RETURN_MAIN_NOT_OK(server.Init(), "Init() failed", 3); - RETURN_MAIN_NOT_OK(server.Start(), "Start() failed", 4); - - while (true) { - SleepFor(MonoDelta::FromSeconds(60)); - } + RETURN_MAIN_NOT_OK(RunMasterServer(), "RunMasterServer() failed", 3); return 0; } diff --git a/src/kudu/master/master_main.cc b/src/kudu/master/master_runner.cc similarity index 50% copy from src/kudu/master/master_main.cc copy to src/kudu/master/master_runner.cc index 7066925..07cec26 100644 --- a/src/kudu/master/master_main.cc +++ b/src/kudu/master/master_runner.cc @@ -15,6 +15,8 @@ // specific language governing permissions and limitations // under the License. +#include "kudu/master/master_runner.h" + #include <iostream> #include <string> @@ -23,95 +25,64 @@ #include "kudu/gutil/strings/substitute.h" #include "kudu/master/master.h" -#include "kudu/util/flag_validators.h" #include "kudu/util/flags.h" -#include "kudu/util/init.h" -#include "kudu/util/logging.h" #include "kudu/util/monotime.h" -#include "kudu/util/status.h" #include "kudu/util/version_info.h" using kudu::master::Master; +using std::string; DECLARE_bool(evict_failed_followers); -DECLARE_int32(webserver_port); -DECLARE_string(rpc_bind_addresses); - -DECLARE_bool(hive_metastore_sasl_enabled); -DECLARE_string(keytab_file); namespace kudu { namespace master { -namespace { -// Validates that if the HMS is configured with SASL enabled, the server has a -// keytab available. This is located in master.cc because the HMS module (where -// -hive_metastore_sasl_enabled is defined) doesn't link to the server module -// (where --keytab_file is defined), and vice-versa. The master module is the -// first module which links to both. -// Note: this check only needs to be run on a server. E.g. tools that run with -// the HMS don't need to pass in a keytab. -bool ValidateHiveMetastoreSaslEnabled() { - if (FLAGS_hive_metastore_sasl_enabled && - FLAGS_keytab_file.empty()) { - LOG(ERROR) << "When the Hive Metastore has SASL enabled " - "(--hive_metastore_sasl_enabled), Kudu must be configured with " - "a keytab (--keytab_file)."; - return false; - } - return true; -} -GROUP_FLAG_VALIDATOR(hive_metastore_sasl_enabled, ValidateHiveMetastoreSaslEnabled); -} // anonymous namespace - -static int MasterMain(int argc, char** argv) { - RETURN_MAIN_NOT_OK(InitKudu(), "InitKudu() failed", 1); - +void SetMasterFlagDefaults() { // Reset some default values before parsing gflags. - FLAGS_rpc_bind_addresses = strings::Substitute("0.0.0.0:$0", - Master::kDefaultPort); - FLAGS_webserver_port = Master::kDefaultWebPort; - - // A multi-node Master leader should not evict failed Master followers - // because there is no-one to assign replacement servers in order to maintain - // the desired replication factor. (It's not turtles all the way down!) - FLAGS_evict_failed_followers = false; + CHECK_NE("", google::SetCommandLineOptionWithMode("rpc_bind_addresses", + strings::Substitute( + "0.0.0.0:$0", + Master::kDefaultPort).c_str(), + google::FlagSettingMode::SET_FLAGS_DEFAULT)); + CHECK_NE("", google::SetCommandLineOptionWithMode("webserver_port", + std::to_string( + Master::kDefaultWebPort).c_str(), + google::FlagSettingMode::SET_FLAGS_DEFAULT)); // Setting the default value of the 'force_block_cache_capacity' flag to // 'false' makes the corresponding group validator enforce proper settings // for the memory limit and the cfile cache capacity. CHECK_NE("", SetCommandLineOptionWithMode("force_block_cache_capacity", - "false", gflags::SET_FLAGS_DEFAULT)); + "false", + gflags::SET_FLAGS_DEFAULT)); - GFlagsMap default_flags = GetFlagsMap(); - - ParseCommandLineFlags(&argc, &argv, true); - if (argc != 1) { - std::cerr << "usage: " << argv[0] << std::endl; - return 2; - } - std::string nondefault_flags = GetNonDefaultFlags(default_flags); - InitGoogleLoggingSafe(argv[0]); + // A multi-node Master leader should not evict failed Master followers + // because there is no-one to assign replacement servers in order to maintain + // the desired replication factor. (It's not turtles all the way down!) + CHECK_NE("", SetCommandLineOptionWithMode("evict_failed_followers", + "false", + gflags::SET_FLAGS_DEFAULT)); + // SET_FLAGS_DEFAULT won't reset the flag value if it has previously been + // set, instead it will only change the default. Because we want to ensure + // evict_failed_followers is always false, we explicitly set the flag. + FLAGS_evict_failed_followers = false; +} +Status RunMasterServer() { + string nondefault_flags = GetNonDefaultFlags(); LOG(INFO) << "Master server non-default flags:\n" << nondefault_flags << '\n' << "Master server version:\n" << VersionInfo::GetAllVersionInfo(); Master server({}); - RETURN_MAIN_NOT_OK(server.Init(), "Init() failed", 3); - RETURN_MAIN_NOT_OK(server.Start(), "Start() failed", 4); + RETURN_NOT_OK(server.Init()); + RETURN_NOT_OK(server.Start()); while (true) { SleepFor(MonoDelta::FromSeconds(60)); } - - return 0; } } // namespace master } // namespace kudu - -int main(int argc, char** argv) { - return kudu::master::MasterMain(argc, argv); -} diff --git a/src/kudu/master/master_runner.h b/src/kudu/master/master_runner.h new file mode 100644 index 0000000..a3fc7cf --- /dev/null +++ b/src/kudu/master/master_runner.h @@ -0,0 +1,31 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +#pragma once + +#include "kudu/util/status.h" + +namespace kudu { +namespace master { + +// Adjusts the default values of some gflags for use with the master server. +void SetMasterFlagDefaults(); + +// Initialize and start a master server. Will run until interrupted. +Status RunMasterServer(); + +} // namespace master +} // namespace kudu diff --git a/src/kudu/server/diagnostics_log.cc b/src/kudu/server/diagnostics_log.cc index 415b5fa..2b11540 100644 --- a/src/kudu/server/diagnostics_log.cc +++ b/src/kudu/server/diagnostics_log.cc @@ -110,8 +110,10 @@ class DiagnosticsLog::SymbolSet { }; DiagnosticsLog::DiagnosticsLog(string log_dir, + string program_name, MetricRegistry* metric_registry) : log_dir_(std::move(log_dir)), + program_name_(std::move(program_name)), metric_registry_(metric_registry), wake_(&lock_), metrics_log_interval_(MonoDelta::FromSeconds(60)), @@ -134,7 +136,7 @@ void DiagnosticsLog::DumpStacksNow(std::string reason) { Status DiagnosticsLog::Start() { - unique_ptr<RollingLog> l(new RollingLog(Env::Default(), log_dir_, "diagnostics")); + unique_ptr<RollingLog> l(new RollingLog(Env::Default(), log_dir_, program_name_, "diagnostics")); RETURN_NOT_OK_PREPEND(l->Open(), "unable to open diagnostics log"); log_ = std::move(l); Status s = Thread::Create("server", "diag-logger", diff --git a/src/kudu/server/diagnostics_log.h b/src/kudu/server/diagnostics_log.h index 5b8cfab..3841007 100644 --- a/src/kudu/server/diagnostics_log.h +++ b/src/kudu/server/diagnostics_log.h @@ -39,7 +39,7 @@ namespace server { class DiagnosticsLog { public: - DiagnosticsLog(std::string log_dir, MetricRegistry* metric_registry); + DiagnosticsLog(std::string log_dir, std::string program_name, MetricRegistry* metric_registry); ~DiagnosticsLog(); void SetMetricsLogInterval(MonoDelta interval); @@ -68,6 +68,7 @@ class DiagnosticsLog { MonoTime ComputeNextWakeup(DiagnosticsLog::WakeupType type) const; const std::string log_dir_; + const std::string program_name_; const MetricRegistry* metric_registry_; scoped_refptr<Thread> thread_; diff --git a/src/kudu/server/server_base.cc b/src/kudu/server/server_base.cc index dcca690..c089eb5 100644 --- a/src/kudu/server/server_base.cc +++ b/src/kudu/server/server_base.cc @@ -225,6 +225,7 @@ DECLARE_bool(use_hybrid_clock); DECLARE_int32(dns_resolver_max_threads_num); DECLARE_uint32(dns_resolver_cache_capacity_mb); DECLARE_uint32(dns_resolver_cache_ttl_sec); +DECLARE_string(log_filename); METRIC_DECLARE_gauge_size(merged_entities_count_of_server); @@ -691,7 +692,8 @@ Status ServerBase::StartMetricsLogging() { LOG(INFO) << "Not starting metrics log since no log directory was specified."; return Status::OK(); } - unique_ptr<DiagnosticsLog> l(new DiagnosticsLog(FLAGS_log_dir, metric_registry_.get())); + unique_ptr<DiagnosticsLog> l(new DiagnosticsLog(FLAGS_log_dir, FLAGS_log_filename, + metric_registry_.get())); l->SetMetricsLogInterval(MonoDelta::FromMilliseconds(options_.metrics_log_interval_ms)); RETURN_NOT_OK(l->Start()); diag_log_ = std::move(l); diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc index 88684f8..0c47957 100644 --- a/src/kudu/tools/kudu-tool-test.cc +++ b/src/kudu/tools/kudu-tool-test.cc @@ -327,11 +327,11 @@ class ToolTest : public KuduTest { ASSERT_TRUE(stdout.empty()); ASSERT_FALSE(stderr.empty()); - // If it was an invalid command, the usage string is on the third line. - int usage_idx = 1; + // If it was an invalid command, the usage string is on the second line. + int usage_idx = 0; if (!expected_status.ok()) { - ASSERT_EQ(expected_status.ToString(), stderr[1]); - usage_idx = 2; + ASSERT_EQ(expected_status.ToString(), stderr[0]); + usage_idx = 1; } ASSERT_EQ(0, stderr[usage_idx].find("Usage: ")); @@ -354,8 +354,8 @@ class ToolTest : public KuduTest { vector<string> err_lines; RunTool(arg_str, nullptr, nullptr, nullptr, /* stderr_lines = */ &err_lines); ASSERT_GE(err_lines.size(), 3) << err_lines; - ASSERT_EQ(expected_status.ToString(), err_lines[1]); - ASSERT_STR_MATCHES(err_lines[3], "Usage: kudu.*"); + ASSERT_EQ(expected_status.ToString(), err_lines[0]); + ASSERT_STR_MATCHES(err_lines[2], "Usage: kudu.*"); } void RunFsCheck(const string& arg_str, diff --git a/src/kudu/tools/tool_action.cc b/src/kudu/tools/tool_action.cc index 5ed62fd..b1ed9ab 100644 --- a/src/kudu/tools/tool_action.cc +++ b/src/kudu/tools/tool_action.cc @@ -33,6 +33,8 @@ #include "kudu/gutil/strings/split.h" #include "kudu/gutil/strings/stringpiece.h" #include "kudu/gutil/strings/substitute.h" +#include "kudu/util/flags.h" +#include "kudu/util/logging.h" #include "kudu/util/url-coding.h" using std::pair; @@ -213,6 +215,12 @@ ActionBuilder& ActionBuilder::ExtraDescription(const string& extra_description) return *this; } +ActionBuilder& ActionBuilder::ProgramName(const string& program_name) { + CHECK(!program_name_.is_initialized()); + program_name_ = program_name; + return *this; +} + ActionBuilder& ActionBuilder::AddRequiredParameter( const ActionArgsDescriptor::Arg& arg) { args_.required.push_back(arg); @@ -246,6 +254,7 @@ unique_ptr<Action> ActionBuilder::Build() { action->name_ = name_; action->description_ = description_; action->extra_description_ = extra_description_; + action->program_name_ = program_name_; action->runner_ = runner_; action->args_ = args_; return action; @@ -255,6 +264,18 @@ Status Action::Run(const vector<Mode*>& chain, const unordered_map<string, string>& required_args, const vector<string>& variadic_args) const { SetOptionalParameterDefaultValues(); + + // If `program_name_` is defined, initialize the logging as if the + // program binary name was `program_name_`, otherwise fallback to the + // default behavior of using argv0. + if (program_name_) { + CHECK_NE("", google::SetCommandLineOptionWithMode("log_filename", + program_name_->c_str(), + google::FlagSettingMode::SET_FLAGS_DEFAULT)); + } + kudu::InitGoogleLoggingSafe(program_name_.get_value_or(gflags::GetArgv0()).c_str()); + kudu::ValidateFlags(); + return runner_({ chain, this, required_args, variadic_args }); } @@ -396,9 +417,9 @@ string Action::BuildHelpXML(const vector<Mode*>& chain) const { void Action::SetOptionalParameterDefaultValues() const { for (const auto& param : args_.optional) { if (param.default_value) { - google::SetCommandLineOptionWithMode(param.name.c_str(), - param.default_value->c_str(), - google::FlagSettingMode::SET_FLAGS_DEFAULT); + CHECK_NE("", google::SetCommandLineOptionWithMode(param.name.c_str(), + param.default_value->c_str(), + google::FlagSettingMode::SET_FLAGS_DEFAULT)); } } } diff --git a/src/kudu/tools/tool_action.h b/src/kudu/tools/tool_action.h index 5b7aace..b8fea5a 100644 --- a/src/kudu/tools/tool_action.h +++ b/src/kudu/tools/tool_action.h @@ -204,6 +204,13 @@ class ActionBuilder { // action's help following Description(). ActionBuilder& ExtraDescription(const std::string& extra_description); + // Sets the program name to use when running this action. If unset, + // argv0 is used. + // + // This will ensure logging is initialized as if the program binary name was + // the passed `program_name`. + ActionBuilder& ProgramName(const std::string& program_name); + // Add a new required parameter to this builder. // // This parameter will be parsed as a positional argument following the name @@ -247,6 +254,8 @@ class ActionBuilder { boost::optional<std::string> extra_description_; + boost::optional<std::string> program_name_; + ActionRunner runner_; ActionArgsDescriptor args_; @@ -284,6 +293,10 @@ class Action { return extra_description_; } + const boost::optional<std::string>& program_name() const { + return program_name_; + } + const ActionArgsDescriptor& args() const { return args_; } private: @@ -301,6 +314,8 @@ class Action { boost::optional<std::string> extra_description_; + boost::optional<std::string> program_name_; + ActionRunner runner_; ActionArgsDescriptor args_; diff --git a/src/kudu/tools/tool_action_master.cc b/src/kudu/tools/tool_action_master.cc index dff62e8..8ed5db1 100644 --- a/src/kudu/tools/tool_action_master.cc +++ b/src/kudu/tools/tool_action_master.cc @@ -28,7 +28,7 @@ #include <boost/algorithm/string/predicate.hpp> #include <boost/optional/optional.hpp> -#include <gflags/gflags_declare.h> +#include <gflags/gflags.h> #include <glog/logging.h> #include "kudu/common/common.pb.h" @@ -43,9 +43,11 @@ #include "kudu/master/master.h" #include "kudu/master/master.pb.h" #include "kudu/master/master.proxy.h" +#include "kudu/master/master_runner.h" #include "kudu/rpc/rpc_controller.h" #include "kudu/tools/tool_action.h" #include "kudu/tools/tool_action_common.h" +#include "kudu/util/init.h" #include "kudu/util/monotime.h" #include "kudu/util/status.h" @@ -87,6 +89,18 @@ Status MasterGetFlags(const RunnerContext& context) { return PrintServerFlags(address, Master::kDefaultPort); } +Status MasterRun(const RunnerContext& context) { + RETURN_NOT_OK(InitKudu()); + + // Enable redaction by default. Unlike most tools, we don't want user data + // printed to the console/log to be shown by default. + CHECK_NE("", google::SetCommandLineOptionWithMode("redact", + "all", google::FlagSettingMode::SET_FLAGS_DEFAULT)); + + master::SetMasterFlagDefaults(); + return master::RunMasterServer(); +} + Status MasterSetFlag(const RunnerContext& context) { const string& address = FindOrDie(context.required_args, kMasterAddressArg); const string& flag = FindOrDie(context.required_args, kFlagArg); @@ -343,6 +357,30 @@ unique_ptr<Mode> BuildMasterMode() { builder.AddAction(std::move(get_flags)); } { + unique_ptr<Action> run = + ActionBuilder("run", &MasterRun) + .ProgramName("kudu-master") + .Description("Runs a Kudu Master") + .ExtraDescription("Note: The master server is started in this process and " + "runs until interrupted.\n\n" + "The most common configuration flags are described below. " + "For all the configuration options pass --helpfull or see " + "https://kudu.apache.org/docs/configuration_reference.html"; + "#kudu-master_supported") + .AddOptionalParameter("master_addresses") + // Even though fs_wal_dir is required, we don't want it to be positional argument. + // This allows it to be passed as a standard flag. + .AddOptionalParameter("fs_wal_dir") + .AddOptionalParameter("fs_data_dirs") + .AddOptionalParameter("fs_metadata_dir") + .AddOptionalParameter("log_dir") + // Unlike most tools we don't log to stderr by default to match the + // kudu-master binary as closely as possible. + .AddOptionalParameter("logtostderr", string("false")) + .Build(); + builder.AddAction(std::move(run)); + } + { unique_ptr<Action> set_flag = ActionBuilder("set_flag", &MasterSetFlag) .Description("Change a gflag value on a Kudu Master") diff --git a/src/kudu/tools/tool_action_tserver.cc b/src/kudu/tools/tool_action_tserver.cc index 337272a..a89a566 100644 --- a/src/kudu/tools/tool_action_tserver.cc +++ b/src/kudu/tools/tool_action_tserver.cc @@ -24,7 +24,7 @@ #include <boost/algorithm/string/predicate.hpp> #include <gflags/gflags.h> -#include <gflags/gflags_declare.h> +#include <glog/logging.h> #include "kudu/common/common.pb.h" #include "kudu/common/wire_protocol.h" @@ -38,6 +38,8 @@ #include "kudu/tools/tool_action.h" #include "kudu/tools/tool_action_common.h" #include "kudu/tserver/tablet_server.h" +#include "kudu/tserver/tablet_server_runner.h" +#include "kudu/util/init.h" #include "kudu/util/status.h" DEFINE_bool(allow_missing_tserver, false, "If true, performs the action on the " @@ -59,6 +61,7 @@ using master::ListTabletServersRequestPB; using master::ListTabletServersResponsePB; using master::MasterServiceProxy; using master::TServerStateChangePB; +using tserver::TabletServer; namespace tools { namespace { @@ -77,6 +80,18 @@ Status TServerGetFlags(const RunnerContext& context) { return PrintServerFlags(address, tserver::TabletServer::kDefaultPort); } +Status TServerRun(const RunnerContext& context) { + RETURN_NOT_OK(InitKudu()); + + // Enable redaction by default. Unlike most tools, we don't want user data + // printed to the console/log to be shown by default. + CHECK_NE("", google::SetCommandLineOptionWithMode("redact", + "all", google::FlagSettingMode::SET_FLAGS_DEFAULT)); + + tserver::SetTabletServerFlagDefaults(); + return tserver::RunTabletServer(); +} + Status TServerSetFlag(const RunnerContext& context) { const string& address = FindOrDie(context.required_args, kTServerAddressArg); const string& flag = FindOrDie(context.required_args, kFlagArg); @@ -229,6 +244,29 @@ unique_ptr<Mode> BuildTServerMode() { .AddOptionalParameter("flag_tags") .Build(); + unique_ptr<Action> run = + ActionBuilder("run", &TServerRun) + .ProgramName("kudu-tserver") + .Description("Runs a Kudu Tablet Server") + .ExtraDescription("Note: The tablet server is started in this process and " + "runs until interrupted.\n\n" + "The most common configuration flags are described below. " + "For all the configuration options pass --helpfull or see " + "https://kudu.apache.org/docs/configuration_reference.html"; + "#kudu-tserver_supported") + .AddOptionalParameter("tserver_master_addrs") + // Even though fs_wal_dir is required, we don't want it to be positional argument. + .AddOptionalParameter("fs_wal_dir") + .AddOptionalParameter("fs_data_dirs") + .AddOptionalParameter("fs_metadata_dir") + .AddOptionalParameter("block_cache_capacity_mb") + .AddOptionalParameter("memory_limit_hard_bytes") + .AddOptionalParameter("log_dir") + // Unlike most tools we don't log to stderr by default to match the + // kudu-tserver binary as closely as possible. + .AddOptionalParameter("logtostderr", string("false")) + .Build(); + unique_ptr<Action> set_flag = ActionBuilder("set_flag", &TServerSetFlag) .Description("Change a gflag value on a Kudu Tablet Server") @@ -291,6 +329,7 @@ unique_ptr<Mode> BuildTServerMode() { .Description("Operate on a Kudu Tablet Server") .AddAction(std::move(dump_memtrackers)) .AddAction(std::move(get_flags)) + .AddAction(std::move(run)) .AddAction(std::move(set_flag)) .AddAction(std::move(status)) .AddAction(std::move(timestamp)) diff --git a/src/kudu/tools/tool_main.cc b/src/kudu/tools/tool_main.cc index 55c5f26..edd190c 100644 --- a/src/kudu/tools/tool_main.cc +++ b/src/kudu/tools/tool_main.cc @@ -26,7 +26,6 @@ #include <boost/optional/optional.hpp> #include <gflags/gflags.h> -#include <gflags/gflags_declare.h> #include <glog/logging.h> #include "kudu/gutil/map-util.h" @@ -34,7 +33,6 @@ #include "kudu/gutil/strings/substitute.h" #include "kudu/tools/tool_action.h" #include "kudu/util/flags.h" -#include "kudu/util/logging.h" #include "kudu/util/path_util.h" #include "kudu/util/status.h" @@ -252,14 +250,16 @@ int main(int argc, char** argv) { CHECK_NE("", google::SetCommandLineOptionWithMode( "redact", "", google::SET_FLAGS_DEFAULT)); + // Set logtostderr by default given these are command line tools. + CHECK_NE("", google::SetCommandLineOptionWithMode( + "logtostderr", "true", google::SET_FLAGS_DEFAULT)); + // Hide the regular gflags help unless --helpfull is used. // // Inspired by https://github.com/gflags/gflags/issues/43#issuecomment-168280647. gflags::ParseCommandLineNonHelpFlags(&argc, &argv, true); - FLAGS_logtostderr = true; const char* prog_name = argv[0]; - kudu::InitGoogleLoggingSafe(prog_name); bool show_help = ParseCommandLineFlags(prog_name); return kudu::tools::RunTool(argc, argv, show_help); diff --git a/src/kudu/tserver/CMakeLists.txt b/src/kudu/tserver/CMakeLists.txt index bcee01a..ae74010 100644 --- a/src/kudu/tserver/CMakeLists.txt +++ b/src/kudu/tserver/CMakeLists.txt @@ -112,6 +112,7 @@ set(TSERVER_SRCS tablet_copy_source_session.cc tablet_server.cc tablet_server_options.cc + tablet_server_runner.cc tablet_service.cc ts_tablet_manager.cc tserver_path_handlers.cc diff --git a/src/kudu/tserver/tablet_server.h b/src/kudu/tserver/tablet_server.h index 5d802c8..bce7773 100644 --- a/src/kudu/tserver/tablet_server.h +++ b/src/kudu/tserver/tablet_server.h @@ -45,6 +45,7 @@ class TabletServer : public kserver::KuduServer { // this constant as well. static const uint16_t kDefaultPort = 7050; static const uint16_t kDefaultWebPort = 8050; + static const uint16_t kDefaultNumServiceThreads = 20; explicit TabletServer(const TabletServerOptions& opts); ~TabletServer(); diff --git a/src/kudu/tserver/tablet_server_main.cc b/src/kudu/tserver/tablet_server_main.cc index c01770a..87de630 100644 --- a/src/kudu/tserver/tablet_server_main.cc +++ b/src/kudu/tserver/tablet_server_main.cc @@ -16,77 +16,28 @@ // under the License. #include <iostream> -#include <string> -#include <gflags/gflags.h> #include <glog/logging.h> -#include "kudu/gutil/macros.h" -#include "kudu/gutil/strings/substitute.h" -#include "kudu/tserver/tablet_server.h" -#include "kudu/util/fault_injection.h" -#include "kudu/util/flag_tags.h" +#include "kudu/tserver/tablet_server_runner.h" #include "kudu/util/flags.h" #include "kudu/util/init.h" #include "kudu/util/logging.h" -#include "kudu/util/monotime.h" #include "kudu/util/status.h" -#include "kudu/util/version_info.h" - -using kudu::tserver::TabletServer; - -DECLARE_int32(rpc_num_service_threads); -DECLARE_int32(webserver_port); -DECLARE_string(rpc_bind_addresses); - -DEFINE_double(fault_before_start, 0.0, - "Fake fault flag that always causes a crash on startup. " - "Used to test the test infrastructure. Should never be set outside of tests."); -TAG_FLAG(fault_before_start, hidden); -TAG_FLAG(fault_before_start, unsafe); namespace kudu { namespace tserver { static int TabletServerMain(int argc, char** argv) { RETURN_MAIN_NOT_OK(InitKudu(), "InitKudu() failed", 1); - - // Reset some default values before parsing gflags. - FLAGS_rpc_bind_addresses = strings::Substitute("0.0.0.0:$0", - TabletServer::kDefaultPort); - FLAGS_rpc_num_service_threads = 20; - FLAGS_webserver_port = TabletServer::kDefaultWebPort; - - // Setting the default value of the 'force_block_cache_capacity' flag to - // 'false' makes the corresponding group validator enforce proper settings - // for the memory limit and the cfile cache capacity. - CHECK_NE("", SetCommandLineOptionWithMode("force_block_cache_capacity", - "false", gflags::SET_FLAGS_DEFAULT)); - - GFlagsMap default_flags = GetFlagsMap(); - + SetTabletServerFlagDefaults(); ParseCommandLineFlags(&argc, &argv, true); if (argc != 1) { std::cerr << "usage: " << argv[0] << std::endl; return 2; } - std::string nondefault_flags = GetNonDefaultFlags(default_flags); InitGoogleLoggingSafe(argv[0]); - - LOG(INFO) << "Tablet server non-default flags:\n" - << nondefault_flags << '\n' - << "Tablet server version:\n" - << VersionInfo::GetAllVersionInfo(); - - TabletServer server({}); - RETURN_MAIN_NOT_OK(server.Init(), "Init() failed", 3); - MAYBE_FAULT(FLAGS_fault_before_start); - RETURN_MAIN_NOT_OK(server.Start(), "Start() failed", 4); - - while (true) { - SleepFor(MonoDelta::FromSeconds(60)); - } - + RETURN_MAIN_NOT_OK(RunTabletServer(), "RunTabletServer() failed", 3); return 0; } diff --git a/src/kudu/tserver/tablet_server_main.cc b/src/kudu/tserver/tablet_server_runner.cc similarity index 67% copy from src/kudu/tserver/tablet_server_main.cc copy to src/kudu/tserver/tablet_server_runner.cc index c01770a..feb7b80 100644 --- a/src/kudu/tserver/tablet_server_main.cc +++ b/src/kudu/tserver/tablet_server_runner.cc @@ -15,6 +15,8 @@ // specific language governing permissions and limitations // under the License. +#include "kudu/tserver/tablet_server_runner.h" + #include <iostream> #include <string> @@ -27,17 +29,11 @@ #include "kudu/util/fault_injection.h" #include "kudu/util/flag_tags.h" #include "kudu/util/flags.h" -#include "kudu/util/init.h" -#include "kudu/util/logging.h" #include "kudu/util/monotime.h" -#include "kudu/util/status.h" #include "kudu/util/version_info.h" using kudu::tserver::TabletServer; - -DECLARE_int32(rpc_num_service_threads); -DECLARE_int32(webserver_port); -DECLARE_string(rpc_bind_addresses); +using std::string; DEFINE_double(fault_before_start, 0.0, "Fake fault flag that always causes a crash on startup. " @@ -48,51 +44,41 @@ TAG_FLAG(fault_before_start, unsafe); namespace kudu { namespace tserver { -static int TabletServerMain(int argc, char** argv) { - RETURN_MAIN_NOT_OK(InitKudu(), "InitKudu() failed", 1); - +void SetTabletServerFlagDefaults() { // Reset some default values before parsing gflags. - FLAGS_rpc_bind_addresses = strings::Substitute("0.0.0.0:$0", - TabletServer::kDefaultPort); - FLAGS_rpc_num_service_threads = 20; - FLAGS_webserver_port = TabletServer::kDefaultWebPort; + CHECK_NE("", google::SetCommandLineOptionWithMode("rpc_bind_addresses", + strings::Substitute("0.0.0.0:$0", TabletServer::kDefaultPort).c_str(), + google::FlagSettingMode::SET_FLAGS_DEFAULT)); + CHECK_NE("", google::SetCommandLineOptionWithMode("rpc_num_service_threads", + std::to_string(TabletServer::kDefaultNumServiceThreads).c_str(), + google::FlagSettingMode::SET_FLAGS_DEFAULT)); + CHECK_NE("", google::SetCommandLineOptionWithMode("webserver_port", + std::to_string(TabletServer::kDefaultWebPort).c_str(), + google::FlagSettingMode::SET_FLAGS_DEFAULT)); // Setting the default value of the 'force_block_cache_capacity' flag to // 'false' makes the corresponding group validator enforce proper settings // for the memory limit and the cfile cache capacity. CHECK_NE("", SetCommandLineOptionWithMode("force_block_cache_capacity", - "false", gflags::SET_FLAGS_DEFAULT)); - - GFlagsMap default_flags = GetFlagsMap(); - - ParseCommandLineFlags(&argc, &argv, true); - if (argc != 1) { - std::cerr << "usage: " << argv[0] << std::endl; - return 2; - } - std::string nondefault_flags = GetNonDefaultFlags(default_flags); - InitGoogleLoggingSafe(argv[0]); + "false", gflags::SET_FLAGS_DEFAULT)); +} +Status RunTabletServer() { + string nondefault_flags = GetNonDefaultFlags(); LOG(INFO) << "Tablet server non-default flags:\n" << nondefault_flags << '\n' << "Tablet server version:\n" << VersionInfo::GetAllVersionInfo(); TabletServer server({}); - RETURN_MAIN_NOT_OK(server.Init(), "Init() failed", 3); + RETURN_NOT_OK(server.Init()); MAYBE_FAULT(FLAGS_fault_before_start); - RETURN_MAIN_NOT_OK(server.Start(), "Start() failed", 4); + RETURN_NOT_OK(server.Start()); while (true) { SleepFor(MonoDelta::FromSeconds(60)); } - - return 0; } } // namespace tserver } // namespace kudu - -int main(int argc, char** argv) { - return kudu::tserver::TabletServerMain(argc, argv); -} diff --git a/src/kudu/tserver/tablet_server_runner.h b/src/kudu/tserver/tablet_server_runner.h new file mode 100644 index 0000000..3b6264e --- /dev/null +++ b/src/kudu/tserver/tablet_server_runner.h @@ -0,0 +1,31 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +#pragma once + +#include "kudu/util/status.h" + +namespace kudu { +namespace tserver { + +// Adjusts the default values of some gflags for use with the tablet server. +void SetTabletServerFlagDefaults(); + +// Initialize and start a tablet server. Will run until interrupted. +Status RunTabletServer(); + +} // namespace tserver +} // namespace kudu diff --git a/src/kudu/util/flag_tags-test.cc b/src/kudu/util/flag_tags-test.cc index 4626d0d..7b7c8a5 100644 --- a/src/kudu/util/flag_tags-test.cc +++ b/src/kudu/util/flag_tags-test.cc @@ -15,18 +15,18 @@ // specific language governing permissions and limitations // under the License. +#include "kudu/util/flag_tags.h" + #include <string> #include <unordered_set> #include <vector> -#include <gtest/gtest.h> #include <gflags/gflags.h> -#include <gflags/gflags_declare.h> +#include <gtest/gtest.h> -#include "kudu/gutil/map-util.h" #include "kudu/gutil/macros.h" +#include "kudu/gutil/map-util.h" #include "kudu/gutil/strings/substitute.h" -#include "kudu/util/flag_tags.h" #include "kudu/util/flags.h" #include "kudu/util/logging.h" #include "kudu/util/logging_test_util.h" @@ -83,7 +83,7 @@ TEST_F(FlagTagsTest, TestUnlockFlags) { { gflags::FlagSaver s; gflags::SetCommandLineOption("test_unsafe_flag", "true"); - ASSERT_DEATH({ HandleCommonFlags(); }, + ASSERT_DEATH({ ValidateFlags(); }, "Flag --test_unsafe_flag is unsafe and unsupported.*" "Use --unlock_unsafe_flags to proceed"); } @@ -95,7 +95,7 @@ TEST_F(FlagTagsTest, TestUnlockFlags) { gflags::FlagSaver s; gflags::SetCommandLineOption("test_unsafe_flag", "true"); gflags::SetCommandLineOption("unlock_unsafe_flags", "true"); - HandleCommonFlags(); + ValidateFlags(); ASSERT_EQ(1, sink.logged_msgs().size()); ASSERT_STR_CONTAINS(sink.logged_msgs()[0], "Enabled unsafe flag: --test_unsafe_flag"); } @@ -104,7 +104,7 @@ TEST_F(FlagTagsTest, TestUnlockFlags) { { gflags::FlagSaver s; gflags::SetCommandLineOption("test_experimental_flag", "true"); - ASSERT_DEATH({ HandleCommonFlags(); }, + ASSERT_DEATH({ ValidateFlags(); }, "Flag --test_experimental_flag is experimental and unsupported.*" "Use --unlock_experimental_flags to proceed"); } @@ -116,7 +116,7 @@ TEST_F(FlagTagsTest, TestUnlockFlags) { gflags::FlagSaver s; gflags::SetCommandLineOption("test_experimental_flag", "true"); gflags::SetCommandLineOption("unlock_experimental_flags", "true"); - HandleCommonFlags(); + ValidateFlags(); ASSERT_EQ(1, sink.logged_msgs().size()); ASSERT_STR_CONTAINS(sink.logged_msgs()[0], "Enabled experimental flag: --test_experimental_flag"); diff --git a/src/kudu/util/flags-test.cc b/src/kudu/util/flags-test.cc index 9ebc178..675ef6f 100644 --- a/src/kudu/util/flags-test.cc +++ b/src/kudu/util/flags-test.cc @@ -15,18 +15,18 @@ // specific language governing permissions and limitations // under the License. +#include "kudu/util/flags.h" + #include <string> #include <vector> #include <gflags/gflags.h> -#include <gflags/gflags_declare.h> #include <glog/logging.h> #include <gtest/gtest.h> #include "kudu/gutil/macros.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/util/env.h" -#include "kudu/util/flags.h" #include "kudu/util/flag_tags.h" #include "kudu/util/logging.h" #include "kudu/util/slice.h" @@ -53,9 +53,6 @@ namespace kudu { class FlagsTest : public KuduTest {}; TEST_F(FlagsTest, TestNonDefaultFlags) { - // Memorize the default flags - GFlagsMap default_flags = GetFlagsMap(); - std::string flagfile_path(GetTestPath("test_nondefault_flags")); std::string flagfile_contents = "--test_nondefault_ff=nondefault\n" "--test_default_ff=default"; @@ -92,7 +89,7 @@ TEST_F(FlagsTest, TestNonDefaultFlags) { // a redacted value. FLAGS_test_sensitive_flag = true; kudu::g_should_redact = kudu::RedactContext::LOG; - std::string result = GetNonDefaultFlags(default_flags); + std::string result = GetNonDefaultFlags(); for (const auto& expected : expected_flags) { ASSERT_STR_CONTAINS(result, expected); diff --git a/src/kudu/util/flags.cc b/src/kudu/util/flags.cc index b966a44..b4eaf7e 100644 --- a/src/kudu/util/flags.cc +++ b/src/kudu/util/flags.cc @@ -17,21 +17,20 @@ #include "kudu/util/flags.h" +#include <sys/stat.h> +#include <unistd.h> // IWYU pragma: keep #include <cstdlib> #include <functional> #include <iostream> +#include <map> #include <string> #include <unordered_set> #include <utility> #include <vector> -#include <sys/stat.h> -#include <unistd.h> // IWYU pragma: keep - #include <boost/algorithm/string/predicate.hpp> #include <gflags/gflags.h> -#include <gflags/gflags_declare.h> #include <glog/logging.h> #ifdef TCMALLOC_ENABLED #include <gperftools/heap-profiler.h> @@ -481,6 +480,7 @@ int ParseCommandLineFlags(int* argc, char*** argv, bool remove_flags) { int ret = google::ParseCommandLineNonHelpFlags(argc, argv, remove_flags); HandleCommonFlags(); + ValidateFlags(); return ret; } @@ -497,8 +497,6 @@ void HandleCommonFlags() { } google::HandleCommandLineHelpFlags(); - CheckFlagsAllowed(); - RunCustomValidators(); if (FLAGS_disable_core_dumps) { DisableCoreDumps(); @@ -527,6 +525,11 @@ void HandleCommonFlags() { #endif } +void ValidateFlags() { + CheckFlagsAllowed(); + RunCustomValidators(); +} + string CommandlineFlagsIntoString(EscapeMode mode) { string ret_value; vector<CommandLineFlagInfo> flags; @@ -546,20 +549,15 @@ string CommandlineFlagsIntoString(EscapeMode mode) { return ret_value; } -string GetNonDefaultFlags(const GFlagsMap& default_flags) { +string GetNonDefaultFlags() { ostringstream args; vector<CommandLineFlagInfo> flags; GetAllFlags(&flags); for (const auto& flag : flags) { if (!flag.is_default) { - // This only means that the flag has been rewritten. It doesn't - // mean that this has been done in the command line, or even - // that it's truly different from the default value. - // Next, we try to check both. - const auto& default_flag = default_flags.find(flag.name); - // it's very unlikely, but still possible that we don't have the flag in defaults - if (default_flag == default_flags.end() || - flag.current_value != default_flag->second.current_value) { + // This only means that the flag has been rewritten. + // We need to check that the value is different from the default value. + if (flag.current_value != flag.default_value) { if (!args.str().empty()) { args << '\n'; } diff --git a/src/kudu/util/flags.h b/src/kudu/util/flags.h index 83cb152..033a57f 100644 --- a/src/kudu/util/flags.h +++ b/src/kudu/util/flags.h @@ -54,6 +54,11 @@ int ParseCommandLineFlags(int* argc, char*** argv, bool remove_flags); // google::ParseCommandLineNonHelpFlags(). void HandleCommonFlags(); +// Verifies that the flags are allowed to be set and valid. +// Should be called after logging is initialized. Otherwise +// logging will write to stderr. +void ValidateFlags(); + enum class EscapeMode { HTML, NONE @@ -70,7 +75,7 @@ typedef std::unordered_map<std::string, google::CommandLineFlagInfo> GFlagsMap; // Get all the flags different from their defaults. The output is a nicely // formatted string with --flag=value pairs per line. Redact any flags that // are tagged as sensitive, if redaction is enabled. -std::string GetNonDefaultFlags(const GFlagsMap& default_flags); +std::string GetNonDefaultFlags(); GFlagsMap GetFlagsMap(); diff --git a/src/kudu/util/logging.cc b/src/kudu/util/logging.cc index cfda466..36850c3 100644 --- a/src/kudu/util/logging.cc +++ b/src/kudu/util/logging.cc @@ -14,6 +14,7 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. + #include "kudu/util/logging.h" #include <unistd.h> @@ -23,10 +24,12 @@ #include <cstdlib> #include <ctime> #include <fstream> +#include <initializer_list> #include <mutex> #include <utility> #include <boost/uuid/random_generator.hpp> +#include <boost/uuid/uuid.hpp> #include <boost/uuid/uuid_io.hpp> #include <gflags/gflags.h> #include <glog/logging.h> @@ -217,13 +220,12 @@ void InitGoogleLoggingSafe(const char* arg) { } } - // This forces our logging to use /tmp rather than looking for a + // This forces our logging to default to /tmp rather than looking for a // temporary directory if none is specified. This is done so that we // can reliably construct the log file name without duplicating the // complex logic that glog uses to guess at a temporary dir. - if (FLAGS_log_dir.empty()) { - FLAGS_log_dir = "/tmp"; - } + CHECK_NE("", google::SetCommandLineOptionWithMode("log_dir", + "/tmp", google::FlagSettingMode::SET_FLAGS_DEFAULT)); if (!FLAGS_logtostderr) { // Verify that a log file can be created in log_dir by creating a tmp file. diff --git a/src/kudu/util/logging.h b/src/kudu/util/logging.h index 0d3492a..f8b03b5 100644 --- a/src/kudu/util/logging.h +++ b/src/kudu/util/logging.h @@ -25,6 +25,7 @@ #include "kudu/gutil/atomicops.h" #include "kudu/gutil/dynamic_annotations.h" #include "kudu/gutil/macros.h" +#include "kudu/gutil/port.h" #include "kudu/gutil/walltime.h" #include "kudu/util/logging_callback.h" #include "kudu/util/status.h" diff --git a/src/kudu/util/minidump.cc b/src/kudu/util/minidump.cc index d02dc5d..9a301f4 100644 --- a/src/kudu/util/minidump.cc +++ b/src/kudu/util/minidump.cc @@ -57,6 +57,7 @@ static constexpr bool kMinidumpPlatformSupported = false; #endif // defined(__linux__) DECLARE_string(log_dir); +DECLARE_string(log_filename); DEFINE_bool(enable_minidumps, kMinidumpPlatformSupported, "Whether to enable minidump generation upon process crash or SIGUSR1. " @@ -208,11 +209,14 @@ Status MinidumpExceptionHandler::InitMinidumpExceptionHandler() { RETURN_NOT_OK_PREPEND(CreateDirIfMissing(env, minidump_dir_), "Error creating top-level minidump directory"); - // Add the executable name to the path where minidumps will be written. This makes - // identification easier and prevents name collisions between the files. + // Add the program_name to the path where minidumps will be written. + // This makes identification easier and prevents name collisions between the files. // This is also consistent with how Impala organizes its minidump files. - const char* exe_name = gflags::ProgramInvocationShortName(); - minidump_dir_ = JoinPathSegments(minidump_dir_, exe_name); + // The log_filename flag will be used if non-empty, otherwise the executable name + // will be used. + const char* program_name = FLAGS_log_filename.empty() ? gflags::ProgramInvocationShortName() : + FLAGS_log_filename.c_str(); + minidump_dir_ = JoinPathSegments(minidump_dir_, program_name); // Create the directory if it is not there. The minidump doesn't get written if there is // no directory. diff --git a/src/kudu/util/rolling_log-test.cc b/src/kudu/util/rolling_log-test.cc index f4f8186..9327ac7 100644 --- a/src/kudu/util/rolling_log-test.cc +++ b/src/kudu/util/rolling_log-test.cc @@ -78,7 +78,7 @@ class RollingLogTest : public KuduTest { // Test with compression off. TEST_F(RollingLogTest, TestLog) { - RollingLog log(env_, log_dir_, "mylog"); + RollingLog log(env_, log_dir_, "rolling_log-test", "mylog"); log.SetCompressionEnabled(false); log.SetRollThresholdBytes(100); @@ -107,7 +107,7 @@ TEST_F(RollingLogTest, TestLog) { // Test with compression on. TEST_F(RollingLogTest, TestCompression) { - RollingLog log(env_, log_dir_, "mylog"); + RollingLog log(env_, log_dir_, "rolling_log-test", "mylog"); ASSERT_OK(log.Open()); StringPiece data = "Hello world\n"; @@ -130,7 +130,7 @@ TEST_F(RollingLogTest, TestCompression) { } TEST_F(RollingLogTest, TestFileCountLimit) { - RollingLog log(env_, log_dir_, "mylog"); + RollingLog log(env_, log_dir_, "rolling_log-test", "mylog"); ASSERT_OK(log.Open()); log.SetRollThresholdBytes(100); log.SetMaxNumSegments(3); diff --git a/src/kudu/util/rolling_log.cc b/src/kudu/util/rolling_log.cc index 610aa9e..9f35e0b 100644 --- a/src/kudu/util/rolling_log.cc +++ b/src/kudu/util/rolling_log.cc @@ -24,10 +24,10 @@ #include <memory> #include <ostream> #include <string> +#include <type_traits> #include <utility> #include <gflags/gflags.h> -#include <gflags/gflags_declare.h> #include <glog/logging.h> #include <zlib.h> @@ -53,9 +53,11 @@ DECLARE_int32(max_log_files); namespace kudu { -RollingLog::RollingLog(Env* env, string log_dir, string log_name) +RollingLog::RollingLog(Env* env, string log_dir, string program_name, string log_name) : env_(env), log_dir_(std::move(log_dir)), + program_name_(program_name.empty() ? + google::ProgramInvocationShortName() : std::move(program_name)), log_name_(std::move(log_name)), roll_threshold_bytes_(kDefaultRollThresholdBytes), max_num_segments_(FLAGS_max_log_files), @@ -121,7 +123,7 @@ string FormattedTimestamp() { string RollingLog::GetLogFileName(int sequence) const { return Substitute("$0.$1.$2.$3.$4.$5.$6", - google::ProgramInvocationShortName(), + program_name_, HostnameOrUnknown(), UsernameOrUnknown(), log_name_, @@ -132,7 +134,7 @@ string RollingLog::GetLogFileName(int sequence) const { string RollingLog::GetLogFilePattern() const { return Substitute("$0.$1.$2.$3.$4.$5.$6", - google::ProgramInvocationShortName(), + program_name_, HostnameOrUnknown(), UsernameOrUnknown(), log_name_, diff --git a/src/kudu/util/rolling_log.h b/src/kudu/util/rolling_log.h index 0bb6755..17ca8e9 100644 --- a/src/kudu/util/rolling_log.h +++ b/src/kudu/util/rolling_log.h @@ -41,7 +41,8 @@ class WritableFile; // // <log_dir>/<program-name>.<hostname>.<user-name>.<log-name>.<timestamp>.<sequence>.<pid> // log_dir: the log_dir specified in the constructor -// program-name: argv[0], as determined by google::ProgramInvocationShortName() +// program-name: the name of the program specified in the constructor. If unset, defaults +// to argv[0], as determined by google::ProgramInvocationShortName(). // hostname: the local machine hostname // user-name: the current user name // log-name: the log_name specified in the constructor @@ -55,7 +56,7 @@ class WritableFile; // This class is not thread-safe and must be externally synchronized. class RollingLog { public: - RollingLog(Env* env, std::string log_dir, std::string log_name); + RollingLog(Env* env, std::string log_dir, std::string program_name, std::string log_name); ~RollingLog(); @@ -111,6 +112,7 @@ class RollingLog { Env* const env_; const std::string log_dir_; + const std::string program_name_; const std::string log_name_; int64_t roll_threshold_bytes_;
