You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/12/28 23:05:48 UTC
[GitHub] [kafka] csolidum opened a new pull request, #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
csolidum opened a new pull request, #13052:
URL: https://github.com/apache/kafka/pull/13052
Now returns an empty Optional instead of throwing a NullPointerException
Added a new test case to cover passing in a null offsetAndMetadata to make sure the works as expected.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] csolidum commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
csolidum commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1370085587
@mimaison @gharris1727 @C0urante
Still getting a sense of what the kafka release process is like for bug fixes. Is there a way to request this patch is included into the next 3.3.X release or the 3.4 release if it's about to be cut?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
mimaison commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1371348167
Yes but I can't merge it in the 3.4 branch at the moment as it's frozen for the release. I'll try to remember to do it once 3.4.0 is out. Feel free to follow up here if you don't see it done.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
mimaison merged PR #13052:
URL: https://github.com/apache/kafka/pull/13052
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a diff in pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #13052:
URL: https://github.com/apache/kafka/pull/13052#discussion_r1060672958
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorCheckpointTask.java:
##########
@@ -189,14 +189,17 @@ private Map<TopicPartition, OffsetAndMetadata> listConsumerGroupOffsets(String g
Optional<Checkpoint> checkpoint(String group, TopicPartition topicPartition,
OffsetAndMetadata offsetAndMetadata) {
- long upstreamOffset = offsetAndMetadata.offset();
- OptionalLong downstreamOffset = offsetSyncStore.translateDownstream(topicPartition, upstreamOffset);
- if (downstreamOffset.isPresent()) {
- return Optional.of(new Checkpoint(group, renameTopicPartition(topicPartition),
+ if (offsetAndMetadata != null) {
+ long upstreamOffset = offsetAndMetadata.offset();
+ OptionalLong downstreamOffset =
+ offsetSyncStore.translateDownstream(topicPartition, upstreamOffset);
+ if (downstreamOffset.isPresent()) {
+ return Optional.of(new Checkpoint(group, renameTopicPartition(topicPartition),
upstreamOffset, downstreamOffset.getAsLong(), offsetAndMetadata.metadata()));
- } else {
- return Optional.empty();
+ }
}
+ return Optional.empty();
+
Review Comment:
nit: Can you remove this unnecessary line?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] csolidum commented on a diff in pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
csolidum commented on code in PR #13052:
URL: https://github.com/apache/kafka/pull/13052#discussion_r1060839550
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorCheckpointTask.java:
##########
@@ -189,14 +189,17 @@ private Map<TopicPartition, OffsetAndMetadata> listConsumerGroupOffsets(String g
Optional<Checkpoint> checkpoint(String group, TopicPartition topicPartition,
OffsetAndMetadata offsetAndMetadata) {
- long upstreamOffset = offsetAndMetadata.offset();
- OptionalLong downstreamOffset = offsetSyncStore.translateDownstream(topicPartition, upstreamOffset);
- if (downstreamOffset.isPresent()) {
- return Optional.of(new Checkpoint(group, renameTopicPartition(topicPartition),
+ if (offsetAndMetadata != null) {
+ long upstreamOffset = offsetAndMetadata.offset();
+ OptionalLong downstreamOffset =
+ offsetSyncStore.translateDownstream(topicPartition, upstreamOffset);
+ if (downstreamOffset.isPresent()) {
+ return Optional.of(new Checkpoint(group, renameTopicPartition(topicPartition),
upstreamOffset, downstreamOffset.getAsLong(), offsetAndMetadata.metadata()));
- } else {
- return Optional.empty();
+ }
}
+ return Optional.empty();
+
Review Comment:
@mimaison 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] csolidum commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
csolidum commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1370118613
@C0urante thanks for the thorough reply. This isn't happening too frequently so i'm fine waiting for the 3.4.1 release. Is the right way to make sure this change is include to email the devs@ mailing list, or is there another process 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1370104415
@csolidum We can backport bug fixes that are low-risk and non-invasive (such as this one) to older branches so that they'll appear in subsequent releases. However, for active releases it's a bit harder to get things in.
For .0 releases (e.g., 3.4.0), if we're past the code freeze deadline (which we are for 3.4.0), then the only fixes we tend to allow on the branch are for either newly-introduced regressions or very serious recently-unearthed issues.
For bug fix releases (e.g., 3.3.2), we don't usually have code freeze deadlines, but we do identify blocker issues that have to be addressed before release candidates can be generated. If all of those issues have been addressed and a release candidate has been generated (which is currently the case for 3.3.2), then unless a blocker issue (which is either a serious recently-unearthed issue, or a newly-introduced regression) is raised, the release will proceed without generating a new candidate.
In this particular case, the bug here doesn't appear to be a regression, and IMO it's not serious enough to qualify as a blocker for the 3.3.2 or 3.4.0 releases. What can probably happen here is that the fix gets backported to the 3.3 and 3.4 branches, but only after the in-flight 3.3.2 and 3.4.0 releases are pushed out. Then, the fix will be present in versions 3.3.3 (if one is released) and 3.4.1.
If that's unsatisfactory, you can reach out on the 3.3.2 and 3.4.0 discussion threads on the dev mailing list and make a case to treat this issue as a blocker by including details on the impact it's having on you/your environment, whether it's a regression (and if not, how old it is), and how much risk it presents.
Full disclosure: I'm the release manager for 3.3.2 and wouldn't mind seeing this added to 3.3.2 (even at the cost of generating a new release candidate), but only if we can also get it included in 3.4.0, to prevent problems from arising should users choose to upgrade from 3.3.2 to 3.4.0.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1370166878
You can just request that the fix be backported on the PR itself. It also helps (but doesn't guarantee a backport) if you populate the `Affects Version/s` field in Jira so that we know which branches contain the bug.
In this case, because we have in-flight releases for 3.3 and 3.4, one other option we have is to mark the Jira ticket as a blocker, and include 3.3.3 and 3.4.1 in the `Fix Version/s` field so that we know it has to be backported before we can put out releases for those versions. It's a bit extreme to label this a blocker but this approach does help us track the need to backport the fix. I'll let @mimaison choose how to handle this, though, since he's the one reviewing (and probably merging) this PR.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] csolidum commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
csolidum commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1367072107
According to the [ci build server](https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-13052/1/tests) test failures existed before this PR
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] csolidum commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
csolidum commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1371337791
@mimaison Thanks for merging. Would it be possible to get this included in the 3.4.1 patch?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
mimaison commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1370753786
I agree, I don't think this qualifies as a blocker for 3.3.2 or 3.4.0. I'll merge in trunk only for now.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13052: KAFKA-14545: Make MirrorCheckpointTask.checkpoint handle null offsetAndMetadata more gracefully
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #13052:
URL: https://github.com/apache/kafka/pull/13052#issuecomment-1370104970
That's the best summary I can provide right now; @mimaison please keep me honest if anything is misleading or incorrect 😄
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org