You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "mneedham (via GitHub)" <gi...@apache.org> on 2023/06/20 10:40:48 UTC
[GitHub] [pinot] mneedham opened a new pull request, #10948: Batch null check
mneedham opened a new pull request, #10948:
URL: https://github.com/apache/pinot/pull/10948
At the moment if you don't specify one of the required job runner class names, you'll end up with a null exception that looks like this:
```
java.lang.RuntimeException: Failed to create IngestionJobRunner instance for class - null
at org.apache.pinot.spi.ingestion.batch.IngestionJobLauncher.kickoffIngestionJob(IngestionJobLauncher.java:145) ~[pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.spi.ingestion.batch.IngestionJobLauncher.runIngestionJob(IngestionJobLauncher.java:130) ~[pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.tools.admin.command.LaunchDataIngestionJobCommand.execute(LaunchDataIngestionJobCommand.java:130) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.tools.Command.call(Command.java:33) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.tools.Command.call(Command.java:29) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at picocli.CommandLine.executeUserObject(CommandLine.java:1953) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at picocli.CommandLine.access$1300(CommandLine.java:145) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2352) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at picocli.CommandLine$RunLast.handle(CommandLine.java:2346) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at picocli.CommandLine$RunLast.handle(CommandLine.java:2311) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2179) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at picocli.CommandLine.execute(CommandLine.java:2078) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.tools.admin.PinotAdministrator.execute(PinotAdministrator.java:171) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.tools.admin.PinotAdministrator.main(PinotAdministrator.java:202) [pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
Caused by: java.lang.NullPointerException
at org.apache.pinot.spi.plugin.PluginManager.createInstance(PluginManager.java:320) ~[pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.spi.plugin.PluginManager.createInstance(PluginManager.java:306) ~[pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
at org.apache.pinot.spi.ingestion.batch.IngestionJobLauncher.kickoffIngestionJob(IngestionJobLauncher.java:143) ~[pinot-all-0.12.0-jar-with-dependencies.jar:0.12.0-118f5e065cb258c171d97a586183759fbc61e2bf]
... 13 more
java.lang.RuntimeException: Failed to create IngestionJobRunner instance for class - null
at org.apache.pinot.spi.ingestion.batch.IngestionJobLauncher.kickoffIngestionJob(IngestionJobLauncher.java:145)
at org.apache.pinot.spi.ingestion.batch.IngestionJobLauncher.runIngestionJob(IngestionJobLauncher.java:130)
at org.apache.pinot.tools.admin.command.LaunchDataIngestionJobCommand.execute(LaunchDataIngestionJobCommand.java:130)
at org.apache.pinot.tools.Command.call(Command.java:33)
at org.apache.pinot.tools.Command.call(Command.java:29)
at picocli.CommandLine.executeUserObject(CommandLine.java:1953)
at picocli.CommandLine.access$1300(CommandLine.java:145)
at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2352)
at picocli.CommandLine$RunLast.handle(CommandLine.java:2346)
at picocli.CommandLine$RunLast.handle(CommandLine.java:2311)
at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:2179)
at picocli.CommandLine.execute(CommandLine.java:2078)
at org.apache.pinot.tools.admin.PinotAdministrator.execute(PinotAdministrator.java:171)
at org.apache.pinot.tools.admin.PinotAdministrator.main(PinotAdministrator.java:202)
Caused by: java.lang.NullPointerException
at org.apache.pinot.spi.plugin.PluginManager.createInstance(PluginManager.java:320)
at org.apache.pinot.spi.plugin.PluginManager.createInstance(PluginManager.java:306)
at org.apache.pinot.spi.ingestion.batch.IngestionJobLauncher.kickoffIngestionJob(IngestionJobLauncher.java:143)
... 13 more
```
This PR adds a null check around the job runner and indicates the name of the missing property.
--
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] Batch null check [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #10948:
URL: https://github.com/apache/pinot/pull/10948#issuecomment-4190883186
Close stale PR, please either rebase and get this PR merged or create a new one.
--
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] Jackie-Jiang commented on a diff in pull request #10948: Batch null check
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10948:
URL: https://github.com/apache/pinot/pull/10948#discussion_r1242497965
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/IngestionJobLauncher.java:
##########
@@ -143,7 +144,7 @@ private static void kickoffIngestionJob(SegmentGenerationJobSpec spec, String in
ingestionJobRunner = PluginManager.get().createInstance(ingestionJobRunnerClassName);
} catch (Exception e) {
throw new RuntimeException(
- "Failed to create IngestionJobRunner instance for class - " + ingestionJobRunnerClassName, e);
+ "Failed to create IngestionJobRunner instance for class - " + ingestionJobRunnerClassName, e);
Review Comment:
Seems this doesn't follow the [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#intellij). Can you please apply this setting and reformat all the changes?
##########
pinot-spi/src/test/resources/ingestion_job_missing_jobrunner.yaml:
##########
@@ -0,0 +1,46 @@
+#
+# 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.
+#
+
+executionFrameworkSpec:
+ name: 'standalone'
+jobType: SegmentCreationAndTarPush
+
+inputDirURI: 'file:///path/to/input/'
+includeFileNamePattern: 'glob:**/*.gz'
+outputDirURI: 'file:///path/to/output/'
+overwriteOutput: true
+segmentCreationJobParallelism: 100
+pinotFSSpecs:
+ - scheme: file
+ className: org.apache.pinot.spi.filesystem.LocalPinotFS
+recordReaderSpec:
+ dataFormat: 'csv'
+ className: 'org.apache.pinot.plugin.inputformat.csv.CSVRecordReader'
+ configClassName: 'org.apache.pinot.plugin.inputformat.csv.CSVRecordReaderConfig'
+ configs:
+ multiValueDelimiter: "\ufff0"
+tableSpec:
+ tableName: 'myTable'
+ schemaURI: 'http://localhost:9000/tables/myTable/schema'
+ tableConfigURI: 'http://localhost:9000/tables/myTable'
+pinotClusterSpecs:
+ - controllerURI: 'localhost:9000'
+pushJobSpec:
+ pushAttempts: 2
+ pushRetryIntervalMillis: 1000
Review Comment:
(minor) empty line in the end
--
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] mneedham commented on a diff in pull request #10948: Batch null check
Posted by "mneedham (via GitHub)" <gi...@apache.org>.
mneedham commented on code in PR #10948:
URL: https://github.com/apache/pinot/pull/10948#discussion_r1238405901
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/IngestionJobLauncher.java:
##########
@@ -136,23 +137,33 @@ public static void runIngestionJob(SegmentGenerationJobSpec spec) {
}
}
- private static void kickoffIngestionJob(SegmentGenerationJobSpec spec, String ingestionJobRunnerClassName) {
- LOGGER.info("Trying to create instance for class {}", ingestionJobRunnerClassName);
+ private static void kickoffIngestionJob(SegmentGenerationJobSpec spec, JobRunner jobRunner) {
+ String jobRunnerClassName = jobRunner.getClassName();
+ LOGGER.info("Trying to create instance for class {}", jobRunnerClassName);
IngestionJobRunner ingestionJobRunner;
try {
- ingestionJobRunner = PluginManager.get().createInstance(ingestionJobRunnerClassName);
+ Preconditions.checkState(
+ jobRunnerClassName != null,
+ jobRunner.getPropertyName() + " in job spec was null"
+ );
+ ingestionJobRunner = PluginManager.get().createInstance(jobRunnerClassName);
} catch (Exception e) {
throw new RuntimeException(
- "Failed to create IngestionJobRunner instance for class - " + ingestionJobRunnerClassName, e);
+ "Failed to create IngestionJobRunner instance for class - " + jobRunnerClassName, e);
}
ingestionJobRunner.init(spec);
try {
ingestionJobRunner.run();
} catch (Exception e) {
- throw new RuntimeException("Caught exception during running - " + ingestionJobRunnerClassName, e);
+ throw new RuntimeException("Caught exception during running - " + jobRunnerClassName, e);
}
}
+ public interface JobRunner {
Review Comment:
Oh yeh, that's a way better idea! I've updated it to use the `NotNull` suffix.
--
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 #10948: Batch null check
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10948:
URL: https://github.com/apache/pinot/pull/10948#issuecomment-1598609936
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10948?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10948](https://app.codecov.io/gh/apache/pinot/pull/10948?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0db4fb6) into [master](https://app.codecov.io/gh/apache/pinot/commit/8b2fb03d8532afd092c0211990e61f156fb45b73?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8b2fb03) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10948 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2188 2134 -54
Lines 117754 115264 -2490
Branches 17795 17494 -301
=========================================
Hits 137 137
+ Misses 117597 115107 -2490
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin17 | `?` | |
| unittests2temurin20 | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10948?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...inot/spi/ingestion/batch/IngestionJobLauncher.java](https://app.codecov.io/gh/apache/pinot/pull/10948?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvaW5nZXN0aW9uL2JhdGNoL0luZ2VzdGlvbkpvYkxhdW5jaGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...i/ingestion/batch/spec/ExecutionFrameworkSpec.java](https://app.codecov.io/gh/apache/pinot/pull/10948?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvaW5nZXN0aW9uL2JhdGNoL3NwZWMvRXhlY3V0aW9uRnJhbWV3b3JrU3BlYy5qYXZh) | `0.00% <0.00%> (ø)` | |
... and [56 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10948/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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] Jackie-Jiang commented on a diff in pull request #10948: Batch null check
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10948:
URL: https://github.com/apache/pinot/pull/10948#discussion_r1237932213
##########
pinot-spi/src/main/java/org/apache/pinot/spi/ingestion/batch/IngestionJobLauncher.java:
##########
@@ -136,23 +137,33 @@ public static void runIngestionJob(SegmentGenerationJobSpec spec) {
}
}
- private static void kickoffIngestionJob(SegmentGenerationJobSpec spec, String ingestionJobRunnerClassName) {
- LOGGER.info("Trying to create instance for class {}", ingestionJobRunnerClassName);
+ private static void kickoffIngestionJob(SegmentGenerationJobSpec spec, JobRunner jobRunner) {
+ String jobRunnerClassName = jobRunner.getClassName();
+ LOGGER.info("Trying to create instance for class {}", jobRunnerClassName);
IngestionJobRunner ingestionJobRunner;
try {
- ingestionJobRunner = PluginManager.get().createInstance(ingestionJobRunnerClassName);
+ Preconditions.checkState(
+ jobRunnerClassName != null,
+ jobRunner.getPropertyName() + " in job spec was null"
+ );
+ ingestionJobRunner = PluginManager.get().createInstance(jobRunnerClassName);
} catch (Exception e) {
throw new RuntimeException(
- "Failed to create IngestionJobRunner instance for class - " + ingestionJobRunnerClassName, e);
+ "Failed to create IngestionJobRunner instance for class - " + jobRunnerClassName, e);
}
ingestionJobRunner.init(spec);
try {
ingestionJobRunner.run();
} catch (Exception e) {
- throw new RuntimeException("Caught exception during running - " + ingestionJobRunnerClassName, e);
+ throw new RuntimeException("Caught exception during running - " + jobRunnerClassName, e);
}
}
+ public interface JobRunner {
Review Comment:
This abstraction doesn't seem clean to me (it is not really a job runner). Can we move the pre-condition check into the getters in `ExecutionFrameworkSpec`? Or we can add checked getters such as `getSegmentGenerationJobRunnerClassNameNotNull()`
--
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] Batch null check [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 closed pull request #10948: Batch null check
URL: https://github.com/apache/pinot/pull/10948
--
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