KAFKA-3641: Fix RecordMetadata constructor backward compatibility#1292
KAFKA-3641: Fix RecordMetadata constructor backward compatibility#1292granthenke wants to merge 1 commit into
Conversation
|
cc @gwenshap |
|
|
||
| @Deprecated | ||
| public RecordMetadata(TopicPartition topicPartition, long baseOffset, long relativeOffset) { | ||
| this(topicPartition, baseOffset, relativeOffset, Record.NO_TIMESTAMP, -1, -1, -1); |
There was a problem hiding this comment.
Thanks. Any chance of adding record.NO_CHECKSUM and record.NO_SIZE?
We have several places in the code where we call the constructor with -1 and I think this will improve readability.
There was a problem hiding this comment.
I can add them if you like. Since they would only be used in the case of a deprecated method (or tests that don't care) I didn't add them thinking the would just be removed when the constructor is removed.
There was a problem hiding this comment.
Does this actually fix compatibility? I don't think we used relative offsets before, so even though it will compile, it won't behave correctly?
There was a problem hiding this comment.
@ijuma The behavior here matches that of 0.9 branch: https://github.com/apache/kafka/blob/0.9.0/clients/src/main/java/org/apache/kafka/clients/producer/RecordMetadata.java#L35
I am not sure how or why people would be using this constructor, perhaps in unit tests or something, but I would prefer to keep it compatible and follow a deprecation cycle since its public.
There was a problem hiding this comment.
A quick search in Github shows a couple places in tests where people use the RecordMetadata constructor (old and new):
- https://github.com/jasperavisser/octowight/blob/bc8b58ab8f3c6f7f860f124e64e010c2d8cc92b7/event-emitter/src/test/scala/nl/haploid/octowight/emitter/service/EventChannelServiceTest.scala#L35
- https://github.com/akka/reactive-kafka/blob/693d4a9d254357e7e3ab98401633b6b3c018a78b/core/src/test/scala/akka/kafka/internal/ProducerTest.scala
- https://github.com/nokia/wookie/blob/76cf9537a3ad835f098c7450ebce918abd35e71f/collector-api/src/test/scala/wookie/collector/cli/KafkaPusherAppSpec.scala
- https://github.com/confluentinc/kafka-rest/blob/b071562041f45ff738f39faff659aebf8eaa8e61/src/test/java/io/confluent/kafkarest/unit/TopicsResourceAvroProduceTest.java#L96
There was a problem hiding this comment.
I agree we should keep it, I was just unsure if semantics had been maintained. Seems like it's fine. Thanks for the references.
|
LGTM. |
Author: Grant Henke <granthenke@gmail.com> Reviewers: Gwen Shapira, Ismael Juma Closes apache#1292 from granthenke/recordmeta-compat
3 participants
No description provided.