Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-5913: Add hasOffset() and hasTimestamp() methods to RecordMetadata #3878

Conversation

apurvam
Copy link
Contributor

@apurvam apurvam commented Sep 16, 2017

We return this exception from RecordMetadata.offset() or RecordMetadata.timestamp() if these pieces of metadata were not returned by the broker.

This will happen, for instance, when the broker returns a DuplicateSequenceException.

@apurvam
Copy link
Contributor Author

apurvam commented Sep 16, 2017

cc @hachikuji @ijuma

I'm open to suggestions for more places where good test cases can be added, if any.

@tedyu
Copy link
Contributor

tedyu commented Sep 16, 2017

lgtm

@ijuma
Copy link
Contributor

ijuma commented Sep 19, 2017

One thing to think about is that the ProducerInterceptor currently has:

* @param metadata The metadata for the record that was sent (i.e. the partition and offset).
     *                 If an error occurred, metadata will contain only valid topic and maybe
     *                 partition. If partition is not given in ProducerRecord and an error occurs
     *                 before partition gets assigned, then partition will be set to RecordMetadata.NO_PARTITION.
     *                 The metadata may be null if the client passed null record to
     *                 {@link org.apache.kafka.clients.producer.KafkaProducer#send(ProducerRecord)}.
     * @param exception The exception thrown during processing of this record. Null if no error occurred.

So, a RecordMetadata with missing data relies on sentinels for some of the fields. I'm not a fan of that approach, but it seems a bit weird for some fields to throw an exception while others throw a sentinel. I haven't thought it in depth though, so maybe there's a good reason to do it that way.

@apurvam
Copy link
Contributor Author

apurvam commented Sep 19, 2017

retest this please

@apurvam
Copy link
Contributor Author

apurvam commented Sep 20, 2017

@ijuma that documentation only applies to cases where an error occurred. In the duplicate sequence case, we don't return an error, but don't return the record metadata either. Perhaps a compromise could be that we only raise the RecordMetadataNotAvailableException in cases where there is no error set, but where we don't have a valid offset and timestamp?

This would be complementary to the expectations of the producer interceptor.

Apurva Mehta added 2 commits September 19, 2017 21:18
RecordMetadata.offset() and RecordMetadata.timestamp(). Instead we
introduce RecordMetadata.hasOffset() and RecordMetadata.hasTimestamp()
methods so that applications can determine whether the result of the
offset() and timestamp() methods is valid.
@apurvam apurvam changed the title KAFKA-5913: Add the RecordMetadataNotAvailableException KAFKA-5913: Add hasOffset() and hasTimestamp() methods to RecordMetadata Sep 20, 2017
@@ -83,7 +83,8 @@ class ProducerFailureHandlingTest extends KafkaServerTestHarness {
}

/**
* With ack == 0 the future metadata will have no exceptions with offset -1
* With ack == 0 we should get a valid record metadata object, but the offset() method should raise
* a RecordMetadataNotAvailableException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this needs to be updated.

@hachikuji
Copy link
Contributor

LGTM. I will merge with a couple trivial cleanups (including the one mentioned by @ijuma).

@asfgit asfgit closed this in ea533f0 Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants