You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/06/01 18:50:41 UTC
[GitHub] [pinot] kkrugler opened a new pull request, #8812: Fix segment generation error handling
kkrugler opened a new pull request, #8812:
URL: https://github.com/apache/pinot/pull/8812
Fix for issue #8641.
Should be tagged with `bugfix`.
Might be considered a backwards-incompatible change, since the stand-alone tool will now fail if there's a segment generation error, while previously it would just log the error and return success.
I also had to fix a few issues with build failures & warnings caused by other commits, thus changes to some `pom.xml` files. Really curious why the dependency convergence failure I get doesn't cause the CI builds to fail.
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r890600454
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -160,6 +162,9 @@ public void init(SegmentGenerationJobSpec spec) {
LOGGER.info("Creating an executor service with {} threads(Job parallelism: {}, available cores: {}.)", numThreads,
jobParallelism, Runtime.getRuntime().availableProcessors());
_executorService = Executors.newFixedThreadPool(numThreads);
+
+ // Set up for recording multiple failures while building segments.
Review Comment:
Good catch, thanks. Fixed.
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888281915
##########
pinot-plugins/pinot-batch-ingestion/v0_deprecated/pinot-spark/pom.xml:
##########
@@ -33,9 +33,9 @@
<url>https://pinot.apache.org/</url>
<properties>
<pinot.root>${basedir}/../../../..</pinot.root>
- <spark.version>2.4.0</spark.version>
- <scala.binary.version>2.11</scala.binary.version>
- <scala.version>2.11.11</scala.version>
+ <spark.version>2.4.7</spark.version>
+ <scala.binary.version>2.12</scala.binary.version>
Review Comment:
I had to bump the Scala version because the version of spark-testing-base that's needed for use with Hadoop 2.10 is only released for 2.12.
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888282485
##########
pinot-plugins/pinot-batch-ingestion/v0_deprecated/pinot-spark/pom.xml:
##########
@@ -33,9 +33,9 @@
<url>https://pinot.apache.org/</url>
<properties>
<pinot.root>${basedir}/../../../..</pinot.root>
- <spark.version>2.4.0</spark.version>
- <scala.binary.version>2.11</scala.binary.version>
- <scala.version>2.11.11</scala.version>
+ <spark.version>2.4.7</spark.version>
+ <scala.binary.version>2.12</scala.binary.version>
Review Comment:
FWIW, Flink recently dropped support for 2.11, partly because too many updated dependencies weren't built against 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] jackjlli commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888219731
##########
pinot-plugins/pinot-batch-ingestion/v0_deprecated/pinot-spark/pom.xml:
##########
@@ -33,9 +33,9 @@
<url>https://pinot.apache.org/</url>
<properties>
<pinot.root>${basedir}/../../../..</pinot.root>
- <spark.version>2.4.0</spark.version>
- <scala.binary.version>2.11</scala.binary.version>
- <scala.version>2.11.11</scala.version>
+ <spark.version>2.4.7</spark.version>
+ <scala.binary.version>2.12</scala.binary.version>
Review Comment:
Do we know why we need to bump up the Scala version in this module? This module is in v0_deprecated module and it shouldn't affect the one in standalone module (correct me if I'm wrong). And directly bumping up Scala version may cause some runtime issue on some of the existing platforms which are running with Scala 2.11.
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888306527
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -194,6 +199,23 @@ public void run()
filteredFiles.add(file);
}
}
+
+ // Sort files based on creation time.
+ Collections.sort(filteredFiles, new Comparator<String>() {
Review Comment:
OK, since input files are sorted by name if `useGlobalDirectorySequenceId` isn't true, I'll rely on that to control ordering of segment generation in the test. I've removed the sort code, and put a comment in about sorting being problematic given the re-ordering that can happen, and load on external file systems.
--
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 #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8812:
URL: https://github.com/apache/pinot/pull/8812#issuecomment-1144040652
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8812?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 [#8812](https://codecov.io/gh/apache/pinot/pull/8812?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (53c2baf) into [master](https://codecov.io/gh/apache/pinot/commit/07b3ee639c6ff75b13465088208287d3297afca1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (07b3ee6) will **decrease** coverage by `48.71%`.
> The diff coverage is `88.88%`.
```diff
@@ Coverage Diff @@
## master #8812 +/- ##
=============================================
- Coverage 62.93% 14.22% -48.72%
+ Complexity 4609 170 -4439
=============================================
Files 1689 1689
Lines 89307 89316 +9
Branches 13415 13416 +1
=============================================
- Hits 56209 12701 -43508
- Misses 29053 75679 +46626
+ Partials 4045 936 -3109
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `14.22% <88.88%> (-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/8812?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/batch/standalone/SegmentGenerationJobRunner.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vcGlub3QtYmF0Y2gtaW5nZXN0aW9uLXN0YW5kYWxvbmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbmdlc3Rpb24vYmF0Y2gvc3RhbmRhbG9uZS9TZWdtZW50R2VuZXJhdGlvbkpvYlJ1bm5lci5qYXZh) | `68.55% <88.88%> (+2.55%)` | :arrow_up: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1130 more](https://codecov.io/gh/apache/pinot/pull/8812/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8812?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8812?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [07b3ee6...53c2baf](https://codecov.io/gh/apache/pinot/pull/8812?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888170974
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -222,6 +229,15 @@ public void run()
}
}
_segmentCreationTaskCountDownLatch.await();
+
+ if (!_failures.isEmpty()) {
+ StringBuilder msg = new StringBuilder("Failed to generate Pinot segment for file(s) - ");
+ _failures.forEach(p -> {
+ msg.append(msg.length() > 0 ? ", " : "");
Review Comment:
This code no longer exists, after I changed things to have a single exception (fail fast) as per our discussion.
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888292079
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -222,6 +244,10 @@ public void run()
}
}
_segmentCreationTaskCountDownLatch.await();
+
+ if (_failure.get() != null) {
Review Comment:
OK - sorry, responded to the previous comment before scrolling down to this 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 merged pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #8812:
URL: https://github.com/apache/pinot/pull/8812
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888291260
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -281,7 +317,11 @@ private void submitSegmentGenTask(File localTempDir, URI inputFileURI, int seqId
_outputDirFS.copyFromLocalFile(localSegmentTarFile, outputSegmentTarURI);
}
} catch (Exception e) {
- LOGGER.error("Failed to generate Pinot segment for file - {}", inputFileURI, e);
+ String msg = "Failed to generate Pinot segment for file - " + inputFileURI.toString();
+ _failure.compareAndSet(null, new RuntimeException(msg, e));
+
+ // We have to decrement the latch by the number of pending tasks.
Review Comment:
1. I haven't seen `shutdownNow()` throwing exceptions due to concurrent task submission - do you have any refs for that?
2. If we don't call `shutdownNow()`, then all submitted segment generation tasks will still be executed when the finally{} code block calls `_executorService.shutdown()`, yes?
--
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 #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888226564
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -222,6 +244,10 @@ public void run()
}
}
_segmentCreationTaskCountDownLatch.await();
+
+ if (_failure.get() != null) {
Review Comment:
We may do `_executorService.shutdownNow()` here. See another comment for details
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -194,6 +199,23 @@ public void run()
filteredFiles.add(file);
}
}
+
+ // Sort files based on creation time.
+ Collections.sort(filteredFiles, new Comparator<String>() {
Review Comment:
Suggest not addressing this TODO in this PR. It can potentially make lots of calls (O(nlogn)) to the PinotFS and cause issues when it is remote. Recently we have encountered issues of hitting the request rate limit for S3.
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -281,7 +317,11 @@ private void submitSegmentGenTask(File localTempDir, URI inputFileURI, int seqId
_outputDirFS.copyFromLocalFile(localSegmentTarFile, outputSegmentTarURI);
}
} catch (Exception e) {
- LOGGER.error("Failed to generate Pinot segment for file - {}", inputFileURI, e);
+ String msg = "Failed to generate Pinot segment for file - " + inputFileURI.toString();
+ _failure.compareAndSet(null, new RuntimeException(msg, e));
+
+ // We have to decrement the latch by the number of pending tasks.
Review Comment:
Suggest shutting down the executor service in the main thread after all the tasks are submitted. It can throw exception if we shut it down here but still trying to submit another task.
We can drain the latch here by checking the remaining count:
```
long count = _segmentCreationTaskCountDownLatch.getCount();
for (int i = 0; i < count; i++) {
_segmentCreationTaskCountDownLatch.countDown();
}
```
--
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 #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r890419704
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -168,7 +173,6 @@ public void run()
//Get list of files to process
String[] files = _inputDirFS.listFiles(_inputDirURI, true);
- //TODO: sort input files based on creation time
Review Comment:
Let's keep this TODO
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -160,6 +162,9 @@ public void init(SegmentGenerationJobSpec spec) {
LOGGER.info("Creating an executor service with {} threads(Job parallelism: {}, available cores: {}.)", numThreads,
jobParallelism, Runtime.getRuntime().availableProcessors());
_executorService = Executors.newFixedThreadPool(numThreads);
+
+ // Set up for recording multiple failures while building segments.
Review Comment:
(minor) The comment is a little bit confusing. Suggest updating it to reflect that we record the first failure
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -253,6 +264,15 @@ private void submitSegmentGenTask(File localTempDir, URI inputFileURI, int seqId
taskSpec.setFailOnEmptySegment(_spec.isFailOnEmptySegment());
taskSpec.setCustomProperty(BatchConfigProperties.INPUT_DATA_FILE_URI_KEY, inputFileURI.toString());
+ // If there's already been a failure, log and skip this file. Do this check right before the
+ // submit to reduce odds of starting a new segment when a failure is recorded right before the
+ // submit.
+ if (_failure.get() != null) {
+ LOGGER.info("Skipping Segment Generation Task for {} due to previous failures", inputFileURI);
+ _segmentCreationTaskCountDownLatch.countDown();
Review Comment:
(minor) This count down is not required because the previous failure should already drain 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] kkrugler commented on pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on PR #8812:
URL: https://github.com/apache/pinot/pull/8812#issuecomment-1146335202
Hi @Jackie-Jiang - let me know if you'd like any further changes, 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.
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888296876
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -194,6 +199,23 @@ public void run()
filteredFiles.add(file);
}
}
+
+ // Sort files based on creation time.
+ Collections.sort(filteredFiles, new Comparator<String>() {
Review Comment:
Hmm, though I do see a `Collections.sort(siblingFiles);` on line 242, wondering if that invalidates the previous sorting...
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -194,6 +199,23 @@ public void run()
filteredFiles.add(file);
}
}
+
+ // Sort files based on creation time.
+ Collections.sort(filteredFiles, new Comparator<String>() {
Review Comment:
Hmm, though I do see a `Collections.sort(siblingFiles);` on line 232, wondering if that invalidates the previous sorting...
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r890599454
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -168,7 +173,6 @@ public void run()
//Get list of files to process
String[] files = _inputDirFS.listFiles(_inputDirURI, true);
- //TODO: sort input files based on creation time
Review Comment:
It's still there (with more details), I'd moved it up to before getting the file list, but moved it back down to reduce confusion.
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888291260
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -281,7 +317,11 @@ private void submitSegmentGenTask(File localTempDir, URI inputFileURI, int seqId
_outputDirFS.copyFromLocalFile(localSegmentTarFile, outputSegmentTarURI);
}
} catch (Exception e) {
- LOGGER.error("Failed to generate Pinot segment for file - {}", inputFileURI, e);
+ String msg = "Failed to generate Pinot segment for file - " + inputFileURI.toString();
+ _failure.compareAndSet(null, new RuntimeException(msg, e));
+
+ // We have to decrement the latch by the number of pending tasks.
Review Comment:
I haven't seen `shutdownNow()` throwing exceptions due to concurrent task submission - do you have any refs for that?
--
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 #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r887304419
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -222,6 +229,15 @@ public void run()
}
}
_segmentCreationTaskCountDownLatch.await();
+
+ if (!_failures.isEmpty()) {
+ StringBuilder msg = new StringBuilder("Failed to generate Pinot segment for file(s) - ");
+ _failures.forEach(p -> {
+ msg.append(msg.length() > 0 ? ", " : "");
Review Comment:
This will always append `", "` because `msg.length()` will never be 0. If we change `_failures` to `Queue<String>`, we can use `StringUtils.join(_failures, ", ")`
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -70,6 +73,7 @@ public class SegmentGenerationJobRunner implements IngestionJobRunner {
private CountDownLatch _segmentCreationTaskCountDownLatch;
private Schema _schema;
private TableConfig _tableConfig;
+ private Queue<Pair<Exception, String>> _failures;
Review Comment:
The `Exception` is not used. Consider changing it to
```suggestion
private Queue<String> _failureFilePaths;
```
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r888284605
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -194,6 +199,23 @@ public void run()
filteredFiles.add(file);
}
}
+
+ // Sort files based on creation time.
+ Collections.sort(filteredFiles, new Comparator<String>() {
Review Comment:
I had to sort files to get deterministic ordering for the test, and since there was a TODO I went ahead and made the change. I can reduce to O(N) by caching the timestamps where we already make the `.isDirectory()` call, lmk if that's an acceptable compromise.
--
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] kkrugler commented on a diff in pull request #8812: Fix segment generation error handling
Posted by GitBox <gi...@apache.org>.
kkrugler commented on code in PR #8812:
URL: https://github.com/apache/pinot/pull/8812#discussion_r890600654
##########
pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java:
##########
@@ -253,6 +264,15 @@ private void submitSegmentGenTask(File localTempDir, URI inputFileURI, int seqId
taskSpec.setFailOnEmptySegment(_spec.isFailOnEmptySegment());
taskSpec.setCustomProperty(BatchConfigProperties.INPUT_DATA_FILE_URI_KEY, inputFileURI.toString());
+ // If there's already been a failure, log and skip this file. Do this check right before the
+ // submit to reduce odds of starting a new segment when a failure is recorded right before the
+ // submit.
+ if (_failure.get() != null) {
+ LOGGER.info("Skipping Segment Generation Task for {} due to previous failures", inputFileURI);
+ _segmentCreationTaskCountDownLatch.countDown();
Review Comment:
Thanks, removed.
--
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