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/23 20:19:11 UTC
[GitHub] [arrow] chrish42 opened a new pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
chrish42 opened a new pull request #7025:
URL: https://github.com/apache/arrow/pull/7025
The following patch ads Gflags support to `plasma-store-server`, leaves out the backtraces on invalid command-line options, and generally tries to make the error messages more useful in terms of nudging the user in the right direction.
Now, calling `plasma-store-server` alone prints `plasma-store-server: please specify socket for incoming connections with -s, and the amount of memory (in bytes) to use with -m`. Likewise with `-h`, although that's a happy coincidence (`-h` was already assigned to turn on hugepage support, and the command-line parsing errors on the missing mandatory options before getting to that).
Calling `plasma-store-server -help` produces:
```
plasma-store-server: Shared-memory server for Arrow data.
Usage:
Flags from ../src/plasma/store.cc:
-d (directory where to create the memory-backed file) type: string
default: "/tmp"
-e (endpoint for external storage service, where objects evicted from
Plasma store can be written to, optional) type: string default: ""
-h (whether to enable hugepage support) type: bool default: false
-m (amount of memory in bytes to use for Plasma store, required)
type: string default: ""
-s (socket name where the Plasma store will listen for requests, required)
type: string default: ""
Flags from /tmp/gflags-20190105-85639-1w01usg/gflags-2.2.2/src/gflags.cc:
-flagfile (load flags from file) type: string default: ""
-fromenv (set flags from the environment [use 'export FLAGS_flag1=value'])
type: string default: ""
-tryfromenv (set flags from the environment if present) type: string
default: ""
-undefok (comma-separated list of flag names that it is okay to specify on
the command line even if the program does not define a flag with that
name. IMPORTANT: flags in this list that have arguments MUST use the
flag=value format) type: string default: ""
Flags from /tmp/gflags-20190105-85639-1w01usg/gflags-2.2.2/src/gflags_completions.cc:
-tab_completion_columns (Number of columns to use in output for tab
completion) type: int32 default: 80
-tab_completion_word (If non-empty, HandleCommandLineCompletions() will
hijack the process and attempt to do bash-style command line flag
completion on this value.) type: string default: ""
Flags from /tmp/gflags-20190105-85639-1w01usg/gflags-2.2.2/src/gflags_reporting.cc:
-help (show help on all flags [tip: all flags can have two dashes])
type: bool default: false currently: true
-helpfull (show help on all flags -- same as -help) type: bool
default: false
-helpmatch (show help on modules whose name contains the specified substr)
type: string default: ""
-helpon (show help on the modules named by this flag value) type: string
default: ""
-helppackage (show help on all modules in the main package) type: bool
default: false
-helpshort (show help on only the main module for this program) type: bool
default: false
-helpxml (produce an xml version of help) type: bool default: false
-version (show version and build info and exit) type: bool default: false
```
which is rather verbose, but at least does the job.
Outstanding questions:
- Is there a way to ask Gflags to suppress all that other stuff? Or do we ditch Gflags and go back to `getopt()` but with a handcrafted help option and message? Or is this good enough?
- I'm assuming we want to be as backward compatible as possible? Otherwise I'd be tempted to move the non-optional command-line options to arguments, and maybe leave `-h` for help.
- In the name of being as backward-compatible as possible, I'm exiting with -1 if the external store directory is not empty, but with 1 for other usage errors? Was that worth the tiny bit of complexity to preserve that behavior?
- Which version define do I use to report `plasma-store-server`'s version?
- Any comments on the description of "Shared-memory server for Arrow data.", or is that good?
----------------------------------------------------------------
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
[GitHub] [arrow] chrish42 commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
chrish42 commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-620679645
Okay, figured out how to run clang-format-8 on the code. (It feels like something that should definitely be easier, especially if it fails the build on the CI.) Let me know if you think anything is missing, because this looks done to me.
----------------------------------------------------------------
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
[GitHub] [arrow] chrish42 commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
chrish42 commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-621243467
@emkornfield I'm chrish42 on the Apache JIRA too. Thanks!
----------------------------------------------------------------
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
[GitHub] [arrow] wesm commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-620886116
@chrish42 you can ignore the Rust lint failures until the Rust nightly linting issues are resolved
----------------------------------------------------------------
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
[GitHub] [arrow] github-actions[bot] commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-618651266
https://issues.apache.org/jira/browse/ARROW-2260
----------------------------------------------------------------
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
[GitHub] [arrow] emkornfield commented on a change in pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#discussion_r414975521
##########
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:
please comment literal parameters /\*parameter_name=\*/
##########
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:
please comment literals with parameter name /*parameter_name=*/
##########
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:
might be worth a comment on why this doesn't use ARROW_LOG(FATAL)
##########
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:
nit: not your code but socket_name != nullptr
##########
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:
replace with UsageError(...)?
##########
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:
"||"?
##########
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:
I'm not actually sure -1 is needed here I don't think the prior code would get past ARROW_LOG(FATAL)?
----------------------------------------------------------------
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
[GitHub] [arrow] emkornfield commented on a change in pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#discussion_r414974208
##########
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:
it seems like this could be done by adding a a define using ARROW_VERSION cmake variable.
----------------------------------------------------------------
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
[GitHub] [arrow] emkornfield commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-619317374
@chrish42 Thank you for the PR, I'll take a look now. Note it looks like lint is failing due to formatting issues. You need to run "make format" or "ninja format" to run clang-format (we are currently using version 8 I believe).
----------------------------------------------------------------
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
[GitHub] [arrow] chrish42 commented on a change in pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [arrow] emkornfield commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-620986493
@chrish42 do you have a JIRA account setup, I'd like to assign the JIRA to you for book-keeping purposes.
----------------------------------------------------------------
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
[GitHub] [arrow] chrish42 commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
chrish42 commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-620771984
Hmm. The dev / lint failure is from `arrow/rust/parquet/src/util/io.rs`, which I didn't touch... 🤔
----------------------------------------------------------------
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
[GitHub] [arrow] emkornfield commented on pull request #7025: ARROW-2260: [C++][Plasma] Use Gflags for command-line parsing
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7025:
URL: https://github.com/apache/arrow/pull/7025#issuecomment-620985725
This seems like a strictly cleaner code, so I'll merge. Thanks @chrish42
----------------------------------------------------------------
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