You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/04/27 19:46:09 UTC
[GitHub] [arrow] chrish42 commented on a change in pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
chrish42 commented on a change in pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#discussion_r416057944
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1207,65 +1211,77 @@ void StartServer(char* socket_name, std::string plasma_directory, bool hugepages
g_runner->Start(socket_name, plasma_directory, hugepages_enabled, external_store);
}
+void UsageError(const char* error_msg, int exit_code=1) {
+ std::cerr << gflags::ProgramInvocationShortName() << ": " << error_msg << std::endl;
+ exit(exit_code);
+}
+
} // namespace plasma
+#ifdef __linux__
+#define SHM_DEFAULT_PATH "/dev/shm"
+#else
+#define SHM_DEFAULT_PATH "/tmp"
+#endif
+
+// Command-line flags.
+DEFINE_string(d, SHM_DEFAULT_PATH, "directory where to create the memory-backed file");
+DEFINE_string(e, "", "endpoint for external storage service, where objects "
+"evicted from Plasma store can be written to, optional");
+DEFINE_bool(h, false, "whether to enable hugepage support");
+DEFINE_string(s, "", "socket name where the Plasma store will listen for requests, required");
+DEFINE_string(m, "", "amount of memory in bytes to use for Plasma store, required");
+
int main(int argc, char* argv[]) {
ArrowLog::StartArrowLog(argv[0], ArrowLogLevel::ARROW_INFO);
ArrowLog::InstallFailureSignalHandler();
+
+ gflags::SetUsageMessage("Shared-memory server for Arrow data.\nUsage: ");
+ gflags::SetVersionString("TODO");
+
char* socket_name = nullptr;
// Directory where plasma memory mapped files are stored.
std::string plasma_directory;
std::string external_store_endpoint;
bool hugepages_enabled = false;
int64_t system_memory = -1;
- int c;
- while ((c = getopt(argc, argv, "s:m:d:e:h")) != -1) {
- switch (c) {
- case 'd':
- plasma_directory = std::string(optarg);
- break;
- case 'e':
- external_store_endpoint = std::string(optarg);
- break;
- case 'h':
- hugepages_enabled = true;
- break;
- case 's':
- socket_name = optarg;
- break;
- case 'm': {
- char extra;
- int scanned = sscanf(optarg, "%" SCNd64 "%c", &system_memory, &extra);
- ARROW_CHECK(scanned == 1);
- // Set system memory capacity
- plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
- ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
- << static_cast<double>(system_memory) / 1000000000
- << "GB of memory.";
- break;
- }
- default:
- exit(-1);
- }
+
+ gflags::ParseCommandLineFlags(&argc, &argv, true);
+ plasma_directory = FLAGS_d;
+ external_store_endpoint = FLAGS_e;
+ hugepages_enabled = FLAGS_h;
+ if (!FLAGS_s.empty()) {
+ // We only check below if socket_name is null, so don't set it if the flag was empty.
+ socket_name = const_cast<char*>(FLAGS_s.c_str());
+ }
+
+ if (!FLAGS_m.empty()) {
+ char extra;
+ int scanned = sscanf(FLAGS_m.c_str(), "%" SCNd64 "%c", &system_memory, &extra);
+ ARROW_CHECK(scanned == 1);
+ // Set system memory capacity
+ plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
+ ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
+ << static_cast<double>(system_memory) / 1000000000
+ << "GB of memory.";
}
+
// Sanity check command line options.
- if (!socket_name) {
- ARROW_LOG(FATAL) << "please specify socket for incoming connections with -s switch";
+ if (!socket_name && system_memory == -1) {
Review comment:
I'm checking here if both are wrong to give a better error message, so people who run the program without arguments get it right the second time, instead of getting an error telling them about `-s`, only to then get a second error telling them about `-m`. But I guess I could add a little comment about that...
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1207,65 +1211,77 @@ void StartServer(char* socket_name, std::string plasma_directory, bool hugepages
g_runner->Start(socket_name, plasma_directory, hugepages_enabled, external_store);
}
+void UsageError(const char* error_msg, int exit_code=1) {
+ std::cerr << gflags::ProgramInvocationShortName() << ": " << error_msg << std::endl;
+ exit(exit_code);
+}
+
} // namespace plasma
+#ifdef __linux__
+#define SHM_DEFAULT_PATH "/dev/shm"
+#else
+#define SHM_DEFAULT_PATH "/tmp"
+#endif
+
+// Command-line flags.
+DEFINE_string(d, SHM_DEFAULT_PATH, "directory where to create the memory-backed file");
+DEFINE_string(e, "", "endpoint for external storage service, where objects "
+"evicted from Plasma store can be written to, optional");
+DEFINE_bool(h, false, "whether to enable hugepage support");
+DEFINE_string(s, "", "socket name where the Plasma store will listen for requests, required");
+DEFINE_string(m, "", "amount of memory in bytes to use for Plasma store, required");
+
int main(int argc, char* argv[]) {
ArrowLog::StartArrowLog(argv[0], ArrowLogLevel::ARROW_INFO);
ArrowLog::InstallFailureSignalHandler();
+
+ gflags::SetUsageMessage("Shared-memory server for Arrow data.\nUsage: ");
+ gflags::SetVersionString("TODO");
Review comment:
There was already an ARROW_VERSION define in "arrow/util/config.h" that was a single number (e.g. 01800), so I added an ARROW_VERSION_STRING define, that is the usual dotted string version number (e.g. "0.18.0").
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1207,65 +1211,77 @@ void StartServer(char* socket_name, std::string plasma_directory, bool hugepages
g_runner->Start(socket_name, plasma_directory, hugepages_enabled, external_store);
}
+void UsageError(const char* error_msg, int exit_code=1) {
+ std::cerr << gflags::ProgramInvocationShortName() << ": " << error_msg << std::endl;
Review comment:
Good point. Added a comment.
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1207,65 +1211,77 @@ void StartServer(char* socket_name, std::string plasma_directory, bool hugepages
g_runner->Start(socket_name, plasma_directory, hugepages_enabled, external_store);
}
+void UsageError(const char* error_msg, int exit_code=1) {
+ std::cerr << gflags::ProgramInvocationShortName() << ": " << error_msg << std::endl;
+ exit(exit_code);
+}
+
} // namespace plasma
+#ifdef __linux__
+#define SHM_DEFAULT_PATH "/dev/shm"
+#else
+#define SHM_DEFAULT_PATH "/tmp"
+#endif
+
+// Command-line flags.
+DEFINE_string(d, SHM_DEFAULT_PATH, "directory where to create the memory-backed file");
+DEFINE_string(e, "", "endpoint for external storage service, where objects "
+"evicted from Plasma store can be written to, optional");
+DEFINE_bool(h, false, "whether to enable hugepage support");
+DEFINE_string(s, "", "socket name where the Plasma store will listen for requests, required");
+DEFINE_string(m, "", "amount of memory in bytes to use for Plasma store, required");
+
int main(int argc, char* argv[]) {
ArrowLog::StartArrowLog(argv[0], ArrowLogLevel::ARROW_INFO);
ArrowLog::InstallFailureSignalHandler();
+
+ gflags::SetUsageMessage("Shared-memory server for Arrow data.\nUsage: ");
+ gflags::SetVersionString("TODO");
+
char* socket_name = nullptr;
// Directory where plasma memory mapped files are stored.
std::string plasma_directory;
std::string external_store_endpoint;
bool hugepages_enabled = false;
int64_t system_memory = -1;
- int c;
- while ((c = getopt(argc, argv, "s:m:d:e:h")) != -1) {
- switch (c) {
- case 'd':
- plasma_directory = std::string(optarg);
- break;
- case 'e':
- external_store_endpoint = std::string(optarg);
- break;
- case 'h':
- hugepages_enabled = true;
- break;
- case 's':
- socket_name = optarg;
- break;
- case 'm': {
- char extra;
- int scanned = sscanf(optarg, "%" SCNd64 "%c", &system_memory, &extra);
- ARROW_CHECK(scanned == 1);
- // Set system memory capacity
- plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
- ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
- << static_cast<double>(system_memory) / 1000000000
- << "GB of memory.";
- break;
- }
- default:
- exit(-1);
- }
+
+ gflags::ParseCommandLineFlags(&argc, &argv, true);
+ plasma_directory = FLAGS_d;
+ external_store_endpoint = FLAGS_e;
+ hugepages_enabled = FLAGS_h;
+ if (!FLAGS_s.empty()) {
+ // We only check below if socket_name is null, so don't set it if the flag was empty.
+ socket_name = const_cast<char*>(FLAGS_s.c_str());
+ }
+
+ if (!FLAGS_m.empty()) {
+ char extra;
+ int scanned = sscanf(FLAGS_m.c_str(), "%" SCNd64 "%c", &system_memory, &extra);
+ ARROW_CHECK(scanned == 1);
Review comment:
Yes, good point. Done.
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1207,65 +1211,77 @@ void StartServer(char* socket_name, std::string plasma_directory, bool hugepages
g_runner->Start(socket_name, plasma_directory, hugepages_enabled, external_store);
}
+void UsageError(const char* error_msg, int exit_code=1) {
+ std::cerr << gflags::ProgramInvocationShortName() << ": " << error_msg << std::endl;
+ exit(exit_code);
+}
+
} // namespace plasma
+#ifdef __linux__
+#define SHM_DEFAULT_PATH "/dev/shm"
+#else
+#define SHM_DEFAULT_PATH "/tmp"
+#endif
+
+// Command-line flags.
+DEFINE_string(d, SHM_DEFAULT_PATH, "directory where to create the memory-backed file");
+DEFINE_string(e, "", "endpoint for external storage service, where objects "
+"evicted from Plasma store can be written to, optional");
+DEFINE_bool(h, false, "whether to enable hugepage support");
+DEFINE_string(s, "", "socket name where the Plasma store will listen for requests, required");
+DEFINE_string(m, "", "amount of memory in bytes to use for Plasma store, required");
+
int main(int argc, char* argv[]) {
ArrowLog::StartArrowLog(argv[0], ArrowLogLevel::ARROW_INFO);
ArrowLog::InstallFailureSignalHandler();
+
+ gflags::SetUsageMessage("Shared-memory server for Arrow data.\nUsage: ");
+ gflags::SetVersionString("TODO");
+
char* socket_name = nullptr;
// Directory where plasma memory mapped files are stored.
std::string plasma_directory;
std::string external_store_endpoint;
bool hugepages_enabled = false;
int64_t system_memory = -1;
- int c;
- while ((c = getopt(argc, argv, "s:m:d:e:h")) != -1) {
- switch (c) {
- case 'd':
- plasma_directory = std::string(optarg);
- break;
- case 'e':
- external_store_endpoint = std::string(optarg);
- break;
- case 'h':
- hugepages_enabled = true;
- break;
- case 's':
- socket_name = optarg;
- break;
- case 'm': {
- char extra;
- int scanned = sscanf(optarg, "%" SCNd64 "%c", &system_memory, &extra);
- ARROW_CHECK(scanned == 1);
- // Set system memory capacity
- plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
- ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
- << static_cast<double>(system_memory) / 1000000000
- << "GB of memory.";
- break;
- }
- default:
- exit(-1);
- }
+
+ gflags::ParseCommandLineFlags(&argc, &argv, true);
+ plasma_directory = FLAGS_d;
+ external_store_endpoint = FLAGS_e;
+ hugepages_enabled = FLAGS_h;
+ if (!FLAGS_s.empty()) {
+ // We only check below if socket_name is null, so don't set it if the flag was empty.
+ socket_name = const_cast<char*>(FLAGS_s.c_str());
+ }
+
+ if (!FLAGS_m.empty()) {
+ char extra;
+ int scanned = sscanf(FLAGS_m.c_str(), "%" SCNd64 "%c", &system_memory, &extra);
+ ARROW_CHECK(scanned == 1);
+ // Set system memory capacity
+ plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
+ ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
+ << static_cast<double>(system_memory) / 1000000000
+ << "GB of memory.";
}
+
// Sanity check command line options.
- if (!socket_name) {
- ARROW_LOG(FATAL) << "please specify socket for incoming connections with -s switch";
+ if (!socket_name && system_memory == -1) {
Review comment:
Added a comment to clarify that this is the "no required switches were passed" case.
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1207,65 +1211,77 @@ void StartServer(char* socket_name, std::string plasma_directory, bool hugepages
g_runner->Start(socket_name, plasma_directory, hugepages_enabled, external_store);
}
+void UsageError(const char* error_msg, int exit_code=1) {
+ std::cerr << gflags::ProgramInvocationShortName() << ": " << error_msg << std::endl;
+ exit(exit_code);
+}
+
} // namespace plasma
+#ifdef __linux__
+#define SHM_DEFAULT_PATH "/dev/shm"
+#else
+#define SHM_DEFAULT_PATH "/tmp"
+#endif
+
+// Command-line flags.
+DEFINE_string(d, SHM_DEFAULT_PATH, "directory where to create the memory-backed file");
+DEFINE_string(e, "", "endpoint for external storage service, where objects "
+"evicted from Plasma store can be written to, optional");
+DEFINE_bool(h, false, "whether to enable hugepage support");
+DEFINE_string(s, "", "socket name where the Plasma store will listen for requests, required");
+DEFINE_string(m, "", "amount of memory in bytes to use for Plasma store, required");
+
int main(int argc, char* argv[]) {
ArrowLog::StartArrowLog(argv[0], ArrowLogLevel::ARROW_INFO);
ArrowLog::InstallFailureSignalHandler();
+
+ gflags::SetUsageMessage("Shared-memory server for Arrow data.\nUsage: ");
+ gflags::SetVersionString("TODO");
+
char* socket_name = nullptr;
// Directory where plasma memory mapped files are stored.
std::string plasma_directory;
std::string external_store_endpoint;
bool hugepages_enabled = false;
int64_t system_memory = -1;
- int c;
- while ((c = getopt(argc, argv, "s:m:d:e:h")) != -1) {
- switch (c) {
- case 'd':
- plasma_directory = std::string(optarg);
- break;
- case 'e':
- external_store_endpoint = std::string(optarg);
- break;
- case 'h':
- hugepages_enabled = true;
- break;
- case 's':
- socket_name = optarg;
- break;
- case 'm': {
- char extra;
- int scanned = sscanf(optarg, "%" SCNd64 "%c", &system_memory, &extra);
- ARROW_CHECK(scanned == 1);
- // Set system memory capacity
- plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
- ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
- << static_cast<double>(system_memory) / 1000000000
- << "GB of memory.";
- break;
- }
- default:
- exit(-1);
- }
+
+ gflags::ParseCommandLineFlags(&argc, &argv, true);
Review comment:
Done.
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1207,65 +1211,77 @@ void StartServer(char* socket_name, std::string plasma_directory, bool hugepages
g_runner->Start(socket_name, plasma_directory, hugepages_enabled, external_store);
}
+void UsageError(const char* error_msg, int exit_code=1) {
+ std::cerr << gflags::ProgramInvocationShortName() << ": " << error_msg << std::endl;
+ exit(exit_code);
+}
+
} // namespace plasma
+#ifdef __linux__
+#define SHM_DEFAULT_PATH "/dev/shm"
+#else
+#define SHM_DEFAULT_PATH "/tmp"
+#endif
+
+// Command-line flags.
+DEFINE_string(d, SHM_DEFAULT_PATH, "directory where to create the memory-backed file");
+DEFINE_string(e, "", "endpoint for external storage service, where objects "
+"evicted from Plasma store can be written to, optional");
+DEFINE_bool(h, false, "whether to enable hugepage support");
+DEFINE_string(s, "", "socket name where the Plasma store will listen for requests, required");
+DEFINE_string(m, "", "amount of memory in bytes to use for Plasma store, required");
+
int main(int argc, char* argv[]) {
ArrowLog::StartArrowLog(argv[0], ArrowLogLevel::ARROW_INFO);
ArrowLog::InstallFailureSignalHandler();
+
+ gflags::SetUsageMessage("Shared-memory server for Arrow data.\nUsage: ");
+ gflags::SetVersionString("TODO");
+
char* socket_name = nullptr;
// Directory where plasma memory mapped files are stored.
std::string plasma_directory;
std::string external_store_endpoint;
bool hugepages_enabled = false;
int64_t system_memory = -1;
- int c;
- while ((c = getopt(argc, argv, "s:m:d:e:h")) != -1) {
- switch (c) {
- case 'd':
- plasma_directory = std::string(optarg);
- break;
- case 'e':
- external_store_endpoint = std::string(optarg);
- break;
- case 'h':
- hugepages_enabled = true;
- break;
- case 's':
- socket_name = optarg;
- break;
- case 'm': {
- char extra;
- int scanned = sscanf(optarg, "%" SCNd64 "%c", &system_memory, &extra);
- ARROW_CHECK(scanned == 1);
- // Set system memory capacity
- plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
- ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
- << static_cast<double>(system_memory) / 1000000000
- << "GB of memory.";
- break;
- }
- default:
- exit(-1);
- }
+
+ gflags::ParseCommandLineFlags(&argc, &argv, true);
+ plasma_directory = FLAGS_d;
+ external_store_endpoint = FLAGS_e;
+ hugepages_enabled = FLAGS_h;
+ if (!FLAGS_s.empty()) {
+ // We only check below if socket_name is null, so don't set it if the flag was empty.
+ socket_name = const_cast<char*>(FLAGS_s.c_str());
+ }
+
+ if (!FLAGS_m.empty()) {
+ char extra;
+ int scanned = sscanf(FLAGS_m.c_str(), "%" SCNd64 "%c", &system_memory, &extra);
+ ARROW_CHECK(scanned == 1);
+ // Set system memory capacity
+ plasma::PlasmaAllocator::SetFootprintLimit(static_cast<size_t>(system_memory));
+ ARROW_LOG(INFO) << "Allowing the Plasma store to use up to "
+ << static_cast<double>(system_memory) / 1000000000
+ << "GB of memory.";
}
+
// Sanity check command line options.
- if (!socket_name) {
- ARROW_LOG(FATAL) << "please specify socket for incoming connections with -s switch";
+ if (!socket_name && system_memory == -1) {
+ plasma::UsageError("please specify socket for incoming connections with -s, "
+ "and the amount of memory (in bytes) to use with -m");
}
- if (system_memory == -1) {
- ARROW_LOG(FATAL) << "please specify the amount of system memory with -m switch";
+ else if (!socket_name) {
Review comment:
Changed, and at a couple other places too in this function.
##########
File path: cpp/src/plasma/store.cc
##########
@@ -1304,8 +1320,9 @@ int main(int argc, char* argv[]) {
plasma::ExternalStores::ExtractStoreName(external_store_endpoint, &name));
external_store = plasma::ExternalStores::GetStore(name);
if (external_store == nullptr) {
- ARROW_LOG(FATAL) << "No such external store \"" << name << "\"";
- return -1;
+ std::ostringstream error_msg;
+ error_msg << "no such external store \"" << name << "\"";
+ plasma::UsageError(error_msg.str().c_str(), -1);
Review comment:
Good point, yes. That simplifies things. I've also renamed the function to `ExitWithUsageError()` so it's unambiguous that it never returns.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org