You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "navina (via GitHub)" <gi...@apache.org> on 2023/05/17 05:27:06 UTC
[GitHub] [pinot] navina opened a new pull request, #10773: Add decoder initialization error to the server's error cache
navina opened a new pull request, #10773:
URL: https://github.com/apache/pinot/pull/10773
Add decoder initialization error to server's error cache so that it can show up in the server's debug API.
A follow-up can be to surface this error in the UI itself.
--
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] navina commented on a diff in pull request #10773: Add decoder initialization error to the server's error cache
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10773:
URL: https://github.com/apache/pinot/pull/10773#discussion_r1196908534
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -1419,8 +1419,15 @@ public LLRealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableCo
// Create message decoder
Set<String> fieldsToRead = IngestionUtils.getFieldsForRecordExtractor(_tableConfig.getIngestionConfig(), _schema);
- StreamMessageDecoder streamMessageDecoder = StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
- _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ try {
+ StreamMessageDecoder streamMessageDecoder =
+ StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
+ _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ } catch (Exception e) {
+ _realtimeTableDataManager.addSegmentError(_segmentNameStr,
+ new SegmentErrorInfo(now(), "Failed to initialize the StreamMessageDecoder", e));
Review Comment:
>yes, what i meant is doing the same thing in this method as how PartitionConsumer is doing in its run method
Given the complexity of this constructor, I wouldn't bucket everything into a single catch block. I think it also helps provide more accurate error message , even before looking at the stacktrace.
--
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] navina commented on a diff in pull request #10773: Add decoder initialization error to the server's error cache
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10773:
URL: https://github.com/apache/pinot/pull/10773#discussion_r1196883043
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -1419,8 +1419,15 @@ public LLRealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableCo
// Create message decoder
Set<String> fieldsToRead = IngestionUtils.getFieldsForRecordExtractor(_tableConfig.getIngestionConfig(), _schema);
- StreamMessageDecoder streamMessageDecoder = StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
- _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ try {
+ StreamMessageDecoder streamMessageDecoder =
+ StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
+ _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ } catch (Exception e) {
+ _realtimeTableDataManager.addSegmentError(_segmentNameStr,
+ new SegmentErrorInfo(now(), "Failed to initialize the StreamMessageDecoder", e));
Review Comment:
Yeah I know what you mean. This class requires a lot of rework.
> there are other _segmentLogger.error or thrown error in this method, don't we also need to call _realtimeTableDataManager.addSegmentError to those as well?
Yeah. I think I can add one more segment error in the last try-catch block in the constructor.
> if the comment above make sense, should we do something similar to PartitionConsumer consume loop and call the _realtimeTableDataManager.addSegmentError at the very end as a catch all?
The partition consumer thread already adds to the segment error cache.
--
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 #10773: Add decoder initialization error to the server's error cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10773:
URL: https://github.com/apache/pinot/pull/10773#discussion_r1196898970
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -1419,8 +1419,15 @@ public LLRealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableCo
// Create message decoder
Set<String> fieldsToRead = IngestionUtils.getFieldsForRecordExtractor(_tableConfig.getIngestionConfig(), _schema);
- StreamMessageDecoder streamMessageDecoder = StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
- _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ try {
+ StreamMessageDecoder streamMessageDecoder =
+ StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
+ _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ } catch (Exception e) {
+ _realtimeTableDataManager.addSegmentError(_segmentNameStr,
+ new SegmentErrorInfo(now(), "Failed to initialize the StreamMessageDecoder", e));
Review Comment:
> The partition consumer thread already adds to the segment error cache.
yes, what i meant is doing the same thing in this method as how PartitionConsumer is doing in its run method
--
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 merged pull request #10773: Add decoder initialization error to the server's error cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10773:
URL: https://github.com/apache/pinot/pull/10773
--
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 #10773: Add decoder initialization error to the server's error cache
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10773:
URL: https://github.com/apache/pinot/pull/10773#issuecomment-1550791710
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10773?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10773](https://app.codecov.io/gh/apache/pinot/pull/10773?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c3017ff) into [master](https://app.codecov.io/gh/apache/pinot/commit/a9c97e30450aaba56869773dd6958399a432ef3b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a9c97e3) will **decrease** coverage by `54.95%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10773 +/- ##
=============================================
- Coverage 68.60% 13.66% -54.95%
+ Complexity 6457 439 -6018
=============================================
Files 2141 2103 -38
Lines 115137 113532 -1605
Branches 17347 17255 -92
=============================================
- Hits 78993 15515 -63478
- Misses 30561 96746 +66185
+ Partials 5583 1271 -4312
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.66% <0.00%> (-0.07%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...manager/realtime/LLRealtimeSegmentDataManager.java](https://app.codecov.io/gh/apache/pinot/pull/10773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvTExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `0.00% <0.00%> (-69.25%)` | :arrow_down: |
... and [1679 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10773/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] walterddr commented on a diff in pull request #10773: Add decoder initialization error to the server's error cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10773:
URL: https://github.com/apache/pinot/pull/10773#discussion_r1196898970
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -1419,8 +1419,15 @@ public LLRealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableCo
// Create message decoder
Set<String> fieldsToRead = IngestionUtils.getFieldsForRecordExtractor(_tableConfig.getIngestionConfig(), _schema);
- StreamMessageDecoder streamMessageDecoder = StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
- _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ try {
+ StreamMessageDecoder streamMessageDecoder =
+ StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
+ _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ } catch (Exception e) {
+ _realtimeTableDataManager.addSegmentError(_segmentNameStr,
+ new SegmentErrorInfo(now(), "Failed to initialize the StreamMessageDecoder", e));
Review Comment:
> The partition consumer thread already adds to the segment error cache.
yes, what i meant is doing the same thing in this method as how PartitionConsumer is doing in its `run` method
--
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 #10773: Add decoder initialization error to the server's error cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10773:
URL: https://github.com/apache/pinot/pull/10773#discussion_r1196823821
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java:
##########
@@ -1419,8 +1419,15 @@ public LLRealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableCo
// Create message decoder
Set<String> fieldsToRead = IngestionUtils.getFieldsForRecordExtractor(_tableConfig.getIngestionConfig(), _schema);
- StreamMessageDecoder streamMessageDecoder = StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
- _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ try {
+ StreamMessageDecoder streamMessageDecoder =
+ StreamDecoderProvider.create(_partitionLevelStreamConfig, fieldsToRead);
+ _streamDataDecoder = new StreamDataDecoderImpl(streamMessageDecoder);
+ } catch (Exception e) {
+ _realtimeTableDataManager.addSegmentError(_segmentNameStr,
+ new SegmentErrorInfo(now(), "Failed to initialize the StreamMessageDecoder", e));
Review Comment:
why do we log to the table data manager here instead of using `_segmentLogger.error` API here?
or the other way to ask is: there are other _segmentLogger.error or thrown error in this method, don't we also need to call `_realtimeTableDataManager.addSegmentError` to those as well?
if the comment above make sense, should we do something similar to `PartitionConsumer` consume loop and call the _realtimeTableDataManager.addSegmentError at the very end as a catch all?
--
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] soumitra-st commented on pull request #10773: Add decoder initialization error to the server's error cache
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on PR #10773:
URL: https://github.com/apache/pinot/pull/10773#issuecomment-1551738351
lgtm
--
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] navina commented on pull request #10773: Add decoder initialization error to the server's error cache
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on PR #10773:
URL: https://github.com/apache/pinot/pull/10773#issuecomment-1551730973
@npawar / @Jackie-Jiang / @mayankshriv : review pls!
--
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