You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2023/02/27 18:52:32 UTC
[GitHub] [druid] gianm opened a new pull request, #13859: Fix NPE in KinesisSupervisor#setupRecordSupplier.
gianm opened a new pull request, #13859:
URL: https://github.com/apache/druid/pull/13859
PR #13539 refactored record supplier creation and introduced a bug: this method would throw NPE when recordsPerFetch was not provided by the user. recordsPerFetch isn't needed in this context at all, since the supervisor-side supplier doesn't fetch records. So this patch sets it to zero.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #13859: Fix NPE in KinesisSupervisor#setupRecordSupplier.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #13859:
URL: https://github.com/apache/druid/pull/13859#issuecomment-1447202470
> Not sure why checkstyle is failing with errors in code that's not touched in this PR:
That's an issue with master, will be fixed by #13860. I'll merge that back here when it's committed.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on pull request #13859: Fix NPE in KinesisSupervisor#setupRecordSupplier.
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on PR #13859:
URL: https://github.com/apache/druid/pull/13859#issuecomment-1446996570
Not sure why checkstyle is failing with errors in code that's not touched in this PR:
```
Error: /home/runner/work/druid/druid/server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java:22:8: Unused import - com.fasterxml.jackson.databind.ObjectMapper. [UnusedImports]
Error: /home/runner/work/druid/druid/server/src/main/java/org/apache/druid/server/coordinator/duty/CompactSegments.java:24:8: Unused import - com.fasterxml.jackson.databind.ObjectMapper. [UnusedImports]
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #13859: Fix NPE in KinesisSupervisor#setupRecordSupplier.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on PR #13859:
URL: https://github.com/apache/druid/pull/13859#issuecomment-1447536138
> Minor suggestion, otherwise LGTM
Thanks for the review. I merged it to avoid the need to re-run CI; would definitely 👍 a PR improving the comment, though.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13859: Fix NPE in KinesisSupervisor#setupRecordSupplier.
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #13859:
URL: https://github.com/apache/druid/pull/13859#discussion_r1119532052
##########
extensions-core/kinesis-indexing-service/src/main/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisor.java:
##########
@@ -197,7 +197,7 @@ protected RecordSupplier<String, String, ByteEntity> setupRecordSupplier() throw
ioConfig.getAwsAssumedRoleArn(),
ioConfig.getAwsExternalId()
),
- ioConfig.getRecordsPerFetch(),
+ 0, // no records-per-fetch, it is not used
Review Comment:
Maybe rephrase the comment for clarity:
```suggestion
0, // need not specify recordsPerFetch as supervisor-side supplier does not fetch records
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #13859: Fix NPE in KinesisSupervisor#setupRecordSupplier.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #13859:
URL: https://github.com/apache/druid/pull/13859
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org