You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jonasgeiregat (via GitHub)" <gi...@apache.org> on 2023/04/23 18:04:10 UTC
[GitHub] [pinot] jonasgeiregat opened a new pull request, #10672: Use the natively supported version and usage by picocli
jonasgeiregat opened a new pull request, #10672:
URL: https://github.com/apache/pinot/pull/10672
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `refactor`
- `cleanup`
The general idea was to make use of the natively supported `mixinStandardHelpOptions` of picocli.
I took the liberty to add some tests for the `PinotAdministrator`.
There are a couple of classes that still need some work but I don't know what would be a good way to tackle it.
I'm referring to:
- `AnonymizeDataCommand`
- `RebalanceTableCommand`
- `RealtimeProvisioningHelperCommand`
All those had a `printExamples` method that is not used anymore, previously this was added to the description when using the `-h` flag.
Now the `-h` flag has been replaced by the native impl. of picocli.
I was thinking of either looking for a way of appending it to the `-h` flag after the native help print out of picocli
or
add another flag --usage which would simply print out everything for those commands.
Amy feedback more then welcome I'm happy to adjust the code in order to get it merged in and improve pinot!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10672: Use the natively supported version and usage by picocli
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10672:
URL: https://github.com/apache/pinot/pull/10672#discussion_r1204589272
##########
pinot-tools/src/test/java/org/apache/pinot/tools/admin/PinotAdministratorTest.java:
##########
@@ -0,0 +1,170 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.tools.admin;
+
+import java.io.StringWriter;
Review Comment:
```suggestion
import com.google.common.collect.ImmutableMap;
import java.io.StringWriter;
```
##########
pinot-tools/src/test/java/org/apache/pinot/tools/admin/PinotAdministratorTest.java:
##########
@@ -0,0 +1,170 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.tools.admin;
+
+import java.io.StringWriter;
+import java.util.Map;
+import java.util.TreeMap;
+import org.apache.pinot.common.Versions;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class PinotAdministratorTest {
+
+ @Test
+ public void testHelpParameterShouldOutputAllPossibleParametersAndTheirDescriptions() {
+ // given
+ StringWriter out = new StringWriter();
+ PinotAdministrator pinotAdministrator = new PinotAdministrator(out, new Versions());
+
+ // when
+ pinotAdministrator.execute(new String[]{"AddSchema", "--help"});
+
+ // then
+ assertEquals(out.toString(), "Usage: <main class> AddSchema [-hV] [-exec] [-authToken=<_authToken>]\n"
+ + " [-authTokenUrl=<_authTokenUrl>]\n"
+ + " [-controllerHost=<_controllerHost>]\n"
+ + " [-controllerPort=<_controllerPort>]\n"
+ + " [-controllerProtocol=<_controllerProtocol>]\n"
+ + " [-password=<_password>] -schemaFile=<_schemaFil"
+ + "e>\n" + " [-user=<_user>]\n"
+ + "Add schema specified in the schema file to the controller\n"
+ + " -authToken=<_authToken>\n" + " Http auth token.\n"
+ + " -authTokenUrl=<_authTokenUrl>\n"
+ + " Http auth token url.\n"
+ + " -controllerHost=<_controllerHost>\n"
+ + " host name for controller.\n"
+ + " -controllerPort=<_controllerPort>\n"
+ + " port name for controller.\n"
+ + " -controllerProtocol=<_controllerProtocol>\n"
+ + " protocol for controller.\n"
+ + " -exec Execute the command.\n"
+ + " -h, --help Show this help message and exit.\n"
+ + " -password=<_password>\n" + " Password for basic auth.\n"
+ + " -schemaFile=<_schemaFile>\n" + " Path to schema file.\n"
+ + " -user=<_user> Username for basic auth.\n"
+ + " -V, --version Print version information and exit.\n");
+ }
+
+ @Test
+ public void testHelp() {
+ StringWriter out = new StringWriter();
+ PinotAdministrator pinotAdministrator = new PinotAdministrator(out, new Versions());
+
+ pinotAdministrator.execute(new String[]{"--help"});
+
+ assertEquals(out.toString(), "Usage: pinot-admin.sh <subCommand>\n" + "Valid subCommands are:\n"
+ + "\t AddTable\t:Create a Pinot table\n"
+ + "\t ChangeTableState\t:Change the state (enable|disable|drop) of Pinot "
+ + "table\n" + "\t DeleteSchema\t:Delete schema specified via name\n"
+ + "\t AddSchema\t:Add schema specified in the schema file to the "
+ + "controller\n"
+ + "\t AnonymizeData\t:Tool to anonymize a given Pinot table data while"
+ + " preserving data characteristics and query patterns\n"
+ + "\t DeleteCluster\t:Remove the Pinot Cluster from Helix.\n"
+ + "\t StartMinion\t:Start the Pinot Minion process at the specified "
+ + "port\n"
+ + "\t VerifyClusterState\t:Verify cluster's state after shutting down "
+ + "several nodes randomly.\n"
+ + "\t LaunchDataIngestionJob\t:Launch a data ingestion job.\n"
+ + "\t StopProcess\t:Stop the specified processes.\n"
+ + "\t ShowClusterInfo\t:Show Pinot Cluster information.\n"
+ + "\t VerifySegmentState\t:Compares helix IdealState and ExternalView for "
+ + "specified table prefixes\n"
+ + "\t QuickStart\t:Launch a complete Pinot cluster within one "
+ + "single process and import pre-built datasets.\n"
+ + "\t StreamGitHubEvents\t:Streams GitHubEvents into a Kafka topic or "
+ + "Kinesis Stream\n"
+ + "\t AddTenant\t:Add tenant as per the specification provided.\n"
+ + "\t StreamAvroIntoKafka\t:Stream the specified Avro file into a Kafka "
+ + "topic, which can be read by Pinot\n"
+ + "by using org.apache.pinot.plugin.stream.kafka.KafkaJSONMessageDecoder as the\n"
+ + "message decoder class name (stream.kafka.decoder.class.name).\n"
+ + "\t JsonToPinotSchema\t:Extracting Pinot schema file from JSON data file"
+ + ".\n" + "\t FileSystem\t:Pinot File system operation.\n"
+ + "\t RealtimeProvisioningHelper\t:Given the table config, partitions, retention "
+ + "and a sample completed segment for a realtime table to be setup, this tool will "
+ + "provide memory used by each host and an optimal segment size for various "
+ + "combinations of hours to consume and hosts. Instead of a completed segment, if "
+ + "schema with characteristics of data is provided, a segment will be generated and "
+ + "used for memory estimation.\n"
+ + "\t RebalanceTable\t:Reassign instances and segments for a table.\n"
+ + "\tCheckOfflineSegmentIntervals\t:Prints out offline segments with invalid time "
+ + "intervals\n"
+ + "\t CreateSegment\t:Create pinot segments from the provided data "
+ + "files.\n"
+ + "\t AvroSchemaToPinotSchema\t:Extracting Pinot schema file from Avro schema or"
+ + " data file.\n"
+ + "\t ValidateConfig\t:Validate configs on the specified Zookeeper.\n"
+ + "\t UploadSegment\t:Upload the Pinot segments.\n"
+ + "\t SegmentProcessorFramework\t:Runs the SegmentProcessorFramework\n"
+ + "\t MoveReplicaGroup\t:Move complete set of segment replica from source"
+ + " servers to tagged servers in cluster\n"
+ + "\t LaunchSparkDataIngestionJob\t:Launch a data ingestion job.\n"
+ + "\t UpdateSchema\t:Add schema specified in the schema file to the "
+ + "controller\n"
+ + "\t StartController\t:Start the Pinot Controller Process at the "
+ + "specified port.\n" + "\t DeleteTable\t:Delete a Pinot table\n"
+ + "\t GenerateData\t:Generate random data as per the provided schema\n"
+ + "\t StartKafka\t:Start Kafka at the specified port.\n"
+ + "\t DataImportDryRun\t:Dry run of data import\n"
+ + "\t BootstrapTable\t:Bootstrap Table.\n"
+ + "\t StartBroker\t:Start the Pinot Broker process at the specified "
+ + "port\n" + "\t ImportData\t:Insert data into Pinot cluster.\n"
+ + "\t PostQuery\t:Query the uploaded Pinot segments.\n"
+ + "\t GitHubEventsQuickStart\t:Runs the GitHubEventsQuickstart\n"
+ + "\t StartZookeeper\t:Start the Zookeeper process at the specified "
+ + "port.\n"
+ + "\t ConvertPinotSegment\t:Convert Pinot segments to another format such as"
+ + " AVRO/CSV/JSON.\n"
+ + "\t OperateClusterConfig\t:Operate Pinot Cluster Config. Sample usage: "
+ + "`pinot-admin.sh OperateClusterConfig -operation DELETE -config pinot.broker"
+ + ".enable.query.limit.override`\n"
+ + "\t StartServer\t:Start the Pinot Server process at the specified "
+ + "port.\n"
+ + "\t StartServiceManager\t:Start the Pinot Service Process at the specified"
+ + " port.\n"
+ + "\t ChangeNumReplicas\t:Re-writes idealState to reflect the value of "
+ + "numReplicas in table config\n"
+ + "For other crud operations, please refer to ${ControllerAddress}/help.\n");
+ }
+
+ @Test
+ public void testVersion() {
+ // given
+ StringWriter out = new StringWriter();
+ PinotAdministrator pinotAdministrator = new PinotAdministrator(out, new VersionsStub());
+
+ // when
+ pinotAdministrator.execute(new String[]{"--version"});
+
+ // then
+ assertEquals(out.toString(), "List All Pinot Component Versions:\n" + "Package: AddSchema, Version: 1.2.3\n"
+ + "Package: AddTable, Version: 1.2.4\n");
+ }
+
+ static class VersionsStub extends Versions {
+ @Override
+ public Map<String, String> getComponentVersions() {
+ return new TreeMap<>(Map.of("AddSchema", "1.2.3", "AddTable", "1.2.4"));
Review Comment:
```suggestion
return new TreeMap<>(ImmutableMap.of("AddSchema", "1.2.3", "AddTable", "1.2.4"));
```
##########
pinot-tools/src/test/java/org/apache/pinot/tools/admin/PinotAdministratorTest.java:
##########
@@ -0,0 +1,170 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.tools.admin;
+
+import java.io.StringWriter;
+import java.util.Map;
+import java.util.TreeMap;
+import org.apache.pinot.common.Versions;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class PinotAdministratorTest {
+
+ @Test
+ public void testHelpParameterShouldOutputAllPossibleParametersAndTheirDescriptions() {
+ // given
+ StringWriter out = new StringWriter();
+ PinotAdministrator pinotAdministrator = new PinotAdministrator(out, new Versions());
+
+ // when
+ pinotAdministrator.execute(new String[]{"AddSchema", "--help"});
+
+ // then
+ assertEquals(out.toString(), "Usage: <main class> AddSchema [-hV] [-exec] [-authToken=<_authToken>]\n"
Review Comment:
this assertEquals is going to be hard to maintain if we add additional commands. was wondering how other project use picocli handles unit-test? do we have a util to generate these expected results?
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/filesystem/MoveFiles.java:
##########
@@ -81,13 +82,8 @@ public boolean execute()
return false;
}
- @Override
- public void printUsage() {
- System.out.println("mv [-o] <source-uri> <destination-uri>");
- }
-
- @Override
- public String description() {
- return "Move file from source to destination, moving from two different PinotFS is not supported.";
- }
+// @Override
+// public void getDescription() {
+// System.out.println("mv [-o] <source-uri> <destination-uri>");
+// }
Review Comment:
remove these comments if no longer needed
##########
pinot-tools/src/main/java/org/apache/pinot/tools/perf/QueryRunner.java:
##########
@@ -107,53 +109,37 @@ private enum QueryMode {
FULL, RESAMPLE
}
- @Override
- public boolean getHelp() {
- return _help;
- }
-
@Override
public String getName() {
return getClass().getSimpleName();
}
- @Override
- public String description() {
- return "Run queries from a query file in singleThread, multiThreads, targetQPS or increasingQPS mode. E.g.\n"
- + " QueryRunner -mode singleThread -queryFile <queryFile> -numTimesToRunQueries 0 "
- + "-numIntervalsToReportAndClearStatistics 5\n"
- + " QueryRunner -mode multiThreads -queryFile <queryFile> -numThreads 10 -reportIntervalMs 1000\n"
- + " QueryRunner -mode targetQPS -queryFile <queryFile> -startQPS 50\n"
- + " QueryRunner -mode increasingQPS -queryFile <queryFile> -startQPS 50 -deltaQPS 10 "
- + "-numIntervalsToIncreaseQPS 20\n";
Review Comment:
did we missed these examples in the new format?
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/RebalanceTableCommand.java:
##########
@@ -111,82 +105,4 @@ public boolean execute()
.info("Got rebalance result: {} for table: {}", JsonUtils.objectToString(rebalanceResult), _tableNameWithType);
return rebalanceResult.getStatus() == RebalanceResult.Status.DONE;
}
-
- @Override
- public String description() {
- return "Reassign instances and segments for a table.";
- }
-
- @Override
- public void printExamples() {
Review Comment:
is this description refactored anywhere in the new code? can't find it
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Use the natively supported version and usage by picocli [pinot]
Posted by "jonasgeiregat (via GitHub)" <gi...@apache.org>.
jonasgeiregat closed pull request #10672: Use the natively supported version and usage by picocli
URL: https://github.com/apache/pinot/pull/10672
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10672: Use the natively supported version and usage by picocli
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10672:
URL: https://github.com/apache/pinot/pull/10672#discussion_r1182908981
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/PinotAdministrator.java:
##########
@@ -140,56 +142,71 @@ public class PinotAdministrator {
SUBCOMMAND_MAP.put("FileSystem", new FileSystemCommand());
}
- @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required = false,
- description = "Print this message.")
+ @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required = false, description = "Print this message.")
private boolean _help = false;
- @CommandLine.Option(names = {"-version", "-v", "--v", "--version"}, required = false,
- description = "Print the version of Pinot package.")
+ @CommandLine.Option(names = {"-version", "-v", "--v", "--version"}, required = false, description = "Print the "
+ + "version of "
+ + "Pinot "
+ + "package.")
private boolean _version = false;
private int _status = 1;
+ private final Writer _outWriter;
+
+ private final Versions _versions;
+
+ public PinotAdministrator(Writer outWriter, Versions versions) {
+ _outWriter = outWriter;
+ _versions = versions;
+ }
+
public void execute(String[] args) {
try {
- CommandLine commandLine = new CommandLine(this);
- for (Map.Entry<String, Command> subCommand : this.getSubCommands().entrySet()) {
- commandLine.addSubcommand(subCommand.getKey(), subCommand.getValue());
- }
- CommandLine.ParseResult parseResult = commandLine.parseArgs(args);
- // TODO: Use the natively supported version and usage by picocli
- // see https://picocli.info/#_mixin_standard_help_options
- if (!parseResult.hasSubcommand()) {
- if (_version) {
- printVersion();
- _status = 0;
- } else if (_help) {
- printUsage();
- _status = 0;
- }
- } else {
+ var commandLine = new CommandLine(this);
Review Comment:
no worries. i wasn't aware either. thank you for quick turn around!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jonasgeiregat commented on pull request #10672: Use the natively supported version and usage by picocli
Posted by "jonasgeiregat (via GitHub)" <gi...@apache.org>.
jonasgeiregat commented on PR #10672:
URL: https://github.com/apache/pinot/pull/10672#issuecomment-1560002140
Saw an attempt was made to built the project but, failed due to incompatibility with java 8.
Think I forgot to actually push my changes 🙈
In any case it should now pass, can you try again ?
Also what is the reason for supporting JDK-8 as a minimum version ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jonasgeiregat commented on a diff in pull request #10672: Use the natively supported version and usage by picocli
Posted by "jonasgeiregat (via GitHub)" <gi...@apache.org>.
jonasgeiregat commented on code in PR #10672:
URL: https://github.com/apache/pinot/pull/10672#discussion_r1182865461
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/PinotAdministrator.java:
##########
@@ -140,56 +142,71 @@ public class PinotAdministrator {
SUBCOMMAND_MAP.put("FileSystem", new FileSystemCommand());
}
- @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required = false,
- description = "Print this message.")
+ @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required = false, description = "Print this message.")
private boolean _help = false;
- @CommandLine.Option(names = {"-version", "-v", "--v", "--version"}, required = false,
- description = "Print the version of Pinot package.")
+ @CommandLine.Option(names = {"-version", "-v", "--v", "--version"}, required = false, description = "Print the "
+ + "version of "
+ + "Pinot "
+ + "package.")
private boolean _version = false;
private int _status = 1;
+ private final Writer _outWriter;
+
+ private final Versions _versions;
+
+ public PinotAdministrator(Writer outWriter, Versions versions) {
+ _outWriter = outWriter;
+ _versions = versions;
+ }
+
public void execute(String[] args) {
try {
- CommandLine commandLine = new CommandLine(this);
- for (Map.Entry<String, Command> subCommand : this.getSubCommands().entrySet()) {
- commandLine.addSubcommand(subCommand.getKey(), subCommand.getValue());
- }
- CommandLine.ParseResult parseResult = commandLine.parseArgs(args);
- // TODO: Use the natively supported version and usage by picocli
- // see https://picocli.info/#_mixin_standard_help_options
- if (!parseResult.hasSubcommand()) {
- if (_version) {
- printVersion();
- _status = 0;
- } else if (_help) {
- printUsage();
- _status = 0;
- }
- } else {
+ var commandLine = new CommandLine(this);
Review Comment:
I wasn't aware my apologies. I removed all JDK8+ specific things.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #10672: Use the natively supported version and usage by picocli
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10672:
URL: https://github.com/apache/pinot/pull/10672#issuecomment-1560135592
> Also what is the reason for supporting JDK-8 as a minimum version ?
I think it was because some users were still on JDK8. we are in the process of removing it (for a while unfortunately)
I will take another review later today. thank you for the contribution
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10672: Use the natively supported version and usage by picocli
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10672:
URL: https://github.com/apache/pinot/pull/10672#issuecomment-1531592876
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10672?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10672](https://codecov.io/gh/apache/pinot/pull/10672?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88f9263) into [master](https://codecov.io/gh/apache/pinot/commit/53cb451ea9bdb9f375bfdb9775348f8d5e7b23c4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (53cb451) will **decrease** coverage by `1.38%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10672 +/- ##
============================================
- Coverage 70.28% 68.91% -1.38%
+ Complexity 6430 6429 -1
============================================
Files 2112 2113 +1
Lines 113994 113996 +2
Branches 17219 17219
============================================
- Hits 80121 78555 -1566
- Misses 28275 29927 +1652
+ Partials 5598 5514 -84
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `24.13% <0.00%> (+0.03%)` | :arrow_up: |
| unittests1 | `67.80% <0.00%> (-0.04%)` | :arrow_down: |
| unittests2 | `13.82% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rc/main/java/org/apache/pinot/common/Versions.java](https://codecov.io/gh/apache/pinot/pull/10672?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vVmVyc2lvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
... and [153 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10672/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jonasgeiregat commented on pull request #10672: Use the natively supported version and usage by picocli
Posted by "jonasgeiregat (via GitHub)" <gi...@apache.org>.
jonasgeiregat commented on PR #10672:
URL: https://github.com/apache/pinot/pull/10672#issuecomment-1530900859
> thank you for the PR, can you clean up the linter issue and submit again.
There seem to be more then checkstyle in place. I suppose I need to get the built green. I'll look into it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jonasgeiregat commented on pull request #10672: Use the natively supported version and usage by picocli
Posted by "jonasgeiregat (via GitHub)" <gi...@apache.org>.
jonasgeiregat commented on PR #10672:
URL: https://github.com/apache/pinot/pull/10672#issuecomment-1530116364
> thank you for the PR, can you clean up the linter issue and submit again.
I ran checkstyle and fixed all open issues.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10672: Use the natively supported version and usage by picocli
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10672:
URL: https://github.com/apache/pinot/pull/10672#discussion_r1182649913
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/PinotAdministrator.java:
##########
@@ -140,56 +142,71 @@ public class PinotAdministrator {
SUBCOMMAND_MAP.put("FileSystem", new FileSystemCommand());
}
- @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required = false,
- description = "Print this message.")
+ @CommandLine.Option(names = {"-help", "-h", "--h", "--help"}, required = false, description = "Print this message.")
private boolean _help = false;
- @CommandLine.Option(names = {"-version", "-v", "--v", "--version"}, required = false,
- description = "Print the version of Pinot package.")
+ @CommandLine.Option(names = {"-version", "-v", "--v", "--version"}, required = false, description = "Print the "
+ + "version of "
+ + "Pinot "
+ + "package.")
private boolean _version = false;
private int _status = 1;
+ private final Writer _outWriter;
+
+ private final Versions _versions;
+
+ public PinotAdministrator(Writer outWriter, Versions versions) {
+ _outWriter = outWriter;
+ _versions = versions;
+ }
+
public void execute(String[] args) {
try {
- CommandLine commandLine = new CommandLine(this);
- for (Map.Entry<String, Command> subCommand : this.getSubCommands().entrySet()) {
- commandLine.addSubcommand(subCommand.getKey(), subCommand.getValue());
- }
- CommandLine.ParseResult parseResult = commandLine.parseArgs(args);
- // TODO: Use the natively supported version and usage by picocli
- // see https://picocli.info/#_mixin_standard_help_options
- if (!parseResult.hasSubcommand()) {
- if (_version) {
- printVersion();
- _status = 0;
- } else if (_help) {
- printUsage();
- _status = 0;
- }
- } else {
+ var commandLine = new CommandLine(this);
Review Comment:
seems like we can't support var b/c of legacy JDK8 support :-(
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org