[
https://issues.apache.org/jira/browse/KAFKA-4635?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ismael Juma updated KAFKA-4635:
-------------------------------
Description:
I collected a number of improvements that I think would be good to do before
the release. [~cmccabe], please correct if I got anything wrong and feel free
to move some items to separate JIRAs.
1. OffsetAndTimestamp is a public class and the javadoc should only include the
behaviour that users will see. The following (or part of it) should probably be
a non-javadoc comment as it only happens internally:
"* The timestamp should never be negative, unless it is invalid. This could
happen when handling a response from a broker that doesn't support KIP-79."
2. There was a bit of a discussion with regards to the name of the exception
that is thrown when a broker is too old. The current name is
ObsoleteBrokerException. We should decide on the name and then we should update
the relevant producer/consumer methods to mention it.
3. [~junrao] suggested that it would be a good idea log when downgrading
requests as the behaviour can be a little different. We should decide the right
logging level and add this.
4. We should have a system test against 0.9.0.1 brokers. We don't support it,
but we should ideally give a reasonable error message.
5. It seems like `Fetcher.listOffset` could use `retrieveOffsetsByTimes`
instead of calling `sendListOffsetRequests` directly. I think that would be a
little better, but not sure if others disagree.
6. [~hachikuji] suggested that a version mismatch in the `offsetsForTimes` call
should result in null entry in map instead of exception for consistency with
how we handle the unsupported message format case. I am adding this to make
sure we discuss it, but I am not actually sure that is what we should do. Under
normal circumstances, the brokers are either too old or not whereas the message
format is a topic level configuration and, strictly speaking, independent of
the broker version (there is a correlation in practice).
7. We log a warning in case of an error while doing an ApiVersions request.
Because it is the first request and we retry, the warning in the log is useful.
We have a similar warning for Metadata requests, but we only did it for
bootstrap brokers. Would it make sense to do the same for ApiVersions?
8. It would be good to add a few more tests for the usable versions
computation. We have a single simple one at the moment.
9. We should add a note to the upgrade notes specifying the change in behaviour
with regards to older broker versions.
cc [~hachikuji].
was:
I collected a number of improvements that I think would be good to do before
the release. [~cmccabe], please correct if I got anything wrong and feel free
to move some items to separate JIRAs.
1. OffsetAndTimestamp is a public class and the javadoc should only include the
behaviour that users will see. The following (or part of it) should probably be
a non-javadoc comment as it only happens internally:
"* The timestamp should never be negative, unless it is invalid. This could
happen when handling a response from a broker that doesn't support KIP-79."
2. There was a bit of a discussion with regards to the name of the exception
that is thrown when a broker is too old. The current name is
ObsoleteBrokerException. We should decide on the name and then we should update
the relevant producer/consumer methods to mention it.
3. [~junrao] suggested that it would be a good idea log when downgrading
requests as the behaviour can be a little different. We should decide the right
logging level and add this.
4. We should have a system test against 0.9.0.1 brokers. We don't support it,
but we should ideally give a reasonable error message.
5. It seems like `Fetcher.listOffset` could use `retrieveOffsetsByTimes`
instead of calling `sendListOffsetRequests` directly. I think that would be a
little better, but not sure if others disagree.
6. [~hachikuji] suggested that a version mismatch in the `offsetsForTimes` call
should result in null entry in map instead of exception for consistency with
how we handle the unsupported message format case. I am adding this to make
sure we discuss it, but I am not actually sure that is what we should do. Under
normal circumstances, the brokers are either too old or not whereas the message
format is a topic level configuration and, strictly speaking, independent of
the broker version (there is a correlation in practice).
7. We log a warning in case of an error while doing an ApiVersions request.
Because it is the first request and we retry, the warning in the log is useful.
We have a similar warning for Metadata requests, but we only did it for
bootstrap brokers. Would it make sense to do the same for ApiVersions?
8. It would be good to add a few more tests for the usable versions
computation. We have a single simple one at the moment.
cc [~hachikuji].
> Client Compatibility follow-up
> ------------------------------
>
> Key: KAFKA-4635
> URL: https://issues.apache.org/jira/browse/KAFKA-4635
> Project: Kafka
> Issue Type: Sub-task
> Components: clients
> Reporter: Ismael Juma
> Assignee: Colin P. McCabe
> Fix For: 0.10.2.0
>
>
> I collected a number of improvements that I think would be good to do before
> the release. [~cmccabe], please correct if I got anything wrong and feel free
> to move some items to separate JIRAs.
> 1. OffsetAndTimestamp is a public class and the javadoc should only include
> the behaviour that users will see. The following (or part of it) should
> probably be a non-javadoc comment as it only happens internally:
> "* The timestamp should never be negative, unless it is invalid. This could
> happen when handling a response from a broker that doesn't support KIP-79."
> 2. There was a bit of a discussion with regards to the name of the exception
> that is thrown when a broker is too old. The current name is
> ObsoleteBrokerException. We should decide on the name and then we should
> update the relevant producer/consumer methods to mention it.
> 3. [~junrao] suggested that it would be a good idea log when downgrading
> requests as the behaviour can be a little different. We should decide the
> right logging level and add this.
> 4. We should have a system test against 0.9.0.1 brokers. We don't support it,
> but we should ideally give a reasonable error message.
> 5. It seems like `Fetcher.listOffset` could use `retrieveOffsetsByTimes`
> instead of calling `sendListOffsetRequests` directly. I think that would be a
> little better, but not sure if others disagree.
> 6. [~hachikuji] suggested that a version mismatch in the `offsetsForTimes`
> call should result in null entry in map instead of exception for consistency
> with how we handle the unsupported message format case. I am adding this to
> make sure we discuss it, but I am not actually sure that is what we should
> do. Under normal circumstances, the brokers are either too old or not whereas
> the message format is a topic level configuration and, strictly speaking,
> independent of the broker version (there is a correlation in practice).
> 7. We log a warning in case of an error while doing an ApiVersions request.
> Because it is the first request and we retry, the warning in the log is
> useful. We have a similar warning for Metadata requests, but we only did it
> for bootstrap brokers. Would it make sense to do the same for ApiVersions?
> 8. It would be good to add a few more tests for the usable versions
> computation. We have a single simple one at the moment.
> 9. We should add a note to the upgrade notes specifying the change in
> behaviour with regards to older broker versions.
> cc [~hachikuji].
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
