You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2017/10/05 04:16:15 UTC
[3/3] kudu git commit: external_mini_cluster: don't pipe daemon
subprocess stdout
external_mini_cluster: don't pipe daemon subprocess stdout
I don't know why ExternalDaemon was configured like this, since nobody
appears to be using the subprocess' stdout pipe. It's actually pretty
dangerous: if the subprocess were to write over 64K of data to stdout, it'd
fill the pipe and the next write to stdout would block indefinitely since no
one is reading from it.
I snuck in some cosmetic changes to {ExternalMiniCluster,MiniKdc}Options.
Change-Id: Iddd8d78cd085975821207d324ea2fd5365a2c2ba
Reviewed-on: http://gerrit.cloudera.org:8080/8187
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/8101bad5
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/8101bad5
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/8101bad5
Branch: refs/heads/master
Commit: 8101bad58a3c4a38182427988ed3fae7bfbc2b5f
Parents: 3f2bf6d
Author: Adar Dembo <ad...@cloudera.com>
Authored: Fri Sep 29 12:42:54 2017 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Thu Oct 5 04:15:06 2017 +0000
----------------------------------------------------------------------
src/kudu/mini-cluster/external_mini_cluster.cc | 10 ++++----
src/kudu/mini-cluster/external_mini_cluster.h | 26 ++++++++++++++-------
src/kudu/security/test/mini_kdc.cc | 10 ++++----
src/kudu/security/test/mini_kdc.h | 5 +++-
4 files changed, 34 insertions(+), 17 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/8101bad5/src/kudu/mini-cluster/external_mini_cluster.cc
----------------------------------------------------------------------
diff --git a/src/kudu/mini-cluster/external_mini_cluster.cc b/src/kudu/mini-cluster/external_mini_cluster.cc
index 896e063..5284275 100644
--- a/src/kudu/mini-cluster/external_mini_cluster.cc
+++ b/src/kudu/mini-cluster/external_mini_cluster.cc
@@ -299,7 +299,7 @@ vector<string> SubstituteInFlags(const vector<string>& orig_flags,
Status ExternalMiniCluster::StartSingleMaster() {
string daemon_id = "master-0";
- ExternalDaemonOptions opts(opts_.logtostderr);
+ ExternalDaemonOptions opts;
opts.messenger = messenger_;
opts.exe = GetBinaryPath(kMasterBinaryName);
opts.wal_dir = GetWalPath(daemon_id);
@@ -311,6 +311,7 @@ Status ExternalMiniCluster::StartSingleMaster() {
}
opts.extra_flags = SubstituteInFlags(opts_.extra_master_flags, 0);
opts.start_process_timeout = opts_.start_process_timeout;
+ opts.logtostderr = opts_.logtostderr;
opts.rpc_bind_address = HostPort(GetBindIpForMaster(0), 0);
scoped_refptr<ExternalMaster> master = new ExternalMaster(opts);
@@ -345,7 +346,7 @@ Status ExternalMiniCluster::StartDistributedMasters() {
for (int i = 0; i < num_masters; i++) {
string daemon_id = Substitute("master-$0", i);
- ExternalDaemonOptions opts(opts_.logtostderr);
+ ExternalDaemonOptions opts;
opts.messenger = messenger_;
opts.exe = exe;
opts.wal_dir = GetWalPath(daemon_id);
@@ -358,6 +359,7 @@ Status ExternalMiniCluster::StartDistributedMasters() {
opts.extra_flags = SubstituteInFlags(flags, i);
opts.start_process_timeout = opts_.start_process_timeout;
opts.rpc_bind_address = peer_hostports[i];
+ opts.logtostderr = opts_.logtostderr;
scoped_refptr<ExternalMaster> peer = new ExternalMaster(opts);
if (opts_.enable_kerberos) {
@@ -393,7 +395,7 @@ Status ExternalMiniCluster::AddTabletServer() {
}
string bind_host = GetBindIpForTabletServer(idx);
- ExternalDaemonOptions opts(opts_.logtostderr);
+ ExternalDaemonOptions opts;
opts.messenger = messenger_;
opts.exe = GetBinaryPath(kTabletServerBinaryName);
opts.wal_dir = GetWalPath(daemon_id);
@@ -406,6 +408,7 @@ Status ExternalMiniCluster::AddTabletServer() {
opts.extra_flags = SubstituteInFlags(opts_.extra_tserver_flags, idx);
opts.start_process_timeout = opts_.start_process_timeout;
opts.rpc_bind_address = HostPort(bind_host, 0);
+ opts.logtostderr = opts_.logtostderr;
scoped_refptr<ExternalTabletServer> ts =
new ExternalTabletServer(opts, master_hostports);
@@ -761,7 +764,6 @@ Status ExternalDaemon::StartProcess(const vector<string>& user_flags) {
// Start the daemon.
unique_ptr<Subprocess> p(new Subprocess(argv));
- p->ShareParentStdout(false);
p->SetEnvVars(extra_env_);
string env_str;
JoinMapKeysAndValues(extra_env_, "=", ",", &env_str);
http://git-wip-us.apache.org/repos/asf/kudu/blob/8101bad5/src/kudu/mini-cluster/external_mini_cluster.h
----------------------------------------------------------------------
diff --git a/src/kudu/mini-cluster/external_mini_cluster.h b/src/kudu/mini-cluster/external_mini_cluster.h
index 4ed96de..05c8106 100644
--- a/src/kudu/mini-cluster/external_mini_cluster.h
+++ b/src/kudu/mini-cluster/external_mini_cluster.h
@@ -76,26 +76,31 @@ struct ExternalMiniClusterOptions {
ExternalMiniClusterOptions();
// Number of masters to start.
- // Default: 1
+ //
+ // Default: 1.
int num_masters;
// Number of TS to start.
- // Default: 1
+ //
+ // Default: 1.
int num_tablet_servers;
// Directory in which to store data.
+ //
// Default: "", which auto-generates a unique path for this cluster.
std::string data_root;
MiniCluster::BindMode bind_mode;
// The path where the kudu daemons should be run from.
+ //
// Default: "", which uses the same path as the currently running executable.
// This works for unit tests, since they all end up in build/latest/bin.
std::string daemon_bin_path;
// Number of data directories to be created for each daemon.
- // Default: 1
+ //
+ // Default: 1.
int num_data_dirs;
// Extra flags for tablet servers and masters respectively.
@@ -120,14 +125,19 @@ struct ExternalMiniClusterOptions {
// Additionally, when the cluster is started, the environment of the
// test process will be modified to include Kerberos credentials for
// a principal named 'testuser'.
+ //
+ // Default: false.
bool enable_kerberos;
- // If true, sends logging output to stderr instead of a log file. Defaults to
- // true.
+ // If true, sends logging output to stderr instead of a log file.
+ //
+ // Default: true.
bool logtostderr;
// Amount of time that may elapse between the creation of a daemon process
- // and the process writing out its info file. Defaults to 30 seconds.
+ // and the process writing out its info file.
+ //
+ // Default: 30s.
MonoDelta start_process_timeout;
};
@@ -334,8 +344,8 @@ class ExternalMiniCluster : public MiniCluster {
};
struct ExternalDaemonOptions {
- explicit ExternalDaemonOptions(bool logtostderr)
- : logtostderr(logtostderr) {
+ ExternalDaemonOptions()
+ : logtostderr(false) {
}
bool logtostderr;
http://git-wip-us.apache.org/repos/asf/kudu/blob/8101bad5/src/kudu/security/test/mini_kdc.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/test/mini_kdc.cc b/src/kudu/security/test/mini_kdc.cc
index 72eed29..95213f5 100644
--- a/src/kudu/security/test/mini_kdc.cc
+++ b/src/kudu/security/test/mini_kdc.cc
@@ -46,7 +46,9 @@ using strings::Substitute;
namespace kudu {
string MiniKdcOptions::ToString() const {
- return strings::Substitute("{ realm: $0, port: $1, data_root: $2 }", realm, port, data_root);
+ return strings::Substitute("{ realm: $0, data_root: $1, port: $2, "
+ "ticket_lifetime: $3, renew_lifetime: $4 }",
+ realm, data_root, port, ticket_lifetime, renew_lifetime);
}
MiniKdc::MiniKdc()
@@ -61,12 +63,12 @@ MiniKdc::MiniKdc(MiniKdcOptions options)
if (options_.data_root.empty()) {
options_.data_root = JoinPathSegments(GetTestDataDirectory(), "krb5kdc");
}
- if (options_.renew_lifetime.empty()) {
- options_.renew_lifetime = "7d";
- }
if (options_.ticket_lifetime.empty()) {
options_.ticket_lifetime = "24h";
}
+ if (options_.renew_lifetime.empty()) {
+ options_.renew_lifetime = "7d";
+ }
}
MiniKdc::~MiniKdc() {
http://git-wip-us.apache.org/repos/asf/kudu/blob/8101bad5/src/kudu/security/test/mini_kdc.h
----------------------------------------------------------------------
diff --git a/src/kudu/security/test/mini_kdc.h b/src/kudu/security/test/mini_kdc.h
index c0d3151..e282cc4 100644
--- a/src/kudu/security/test/mini_kdc.h
+++ b/src/kudu/security/test/mini_kdc.h
@@ -36,15 +36,18 @@ class Subprocess;
struct MiniKdcOptions {
// Kerberos Realm.
- // Default: "KRBTEST.COM"
+ //
+ // Default: "KRBTEST.COM".
std::string realm;
// Directory in which to store data.
+ //
// Default: "", which auto-generates a unique path for this KDC.
// The default may only be used from a gtest unit test.
std::string data_root;
// KDC port.
+ //
// Default: 0 (ephemeral port).
uint16_t port = 0;