Skip to content

KAFKA-15859: Fixed the Unsupported version error when new admin connects to old broker#17358

Merged
kamalcph merged 1 commit intoapache:trunkfrom
kamalcph:KAFKA-15859d
Oct 4, 2024
Merged

KAFKA-15859: Fixed the Unsupported version error when new admin connects to old broker#17358
kamalcph merged 1 commit intoapache:trunkfrom
kamalcph:KAFKA-15859d

Conversation

@kamalcph
Copy link
Copy Markdown
Contributor

@kamalcph kamalcph commented Oct 3, 2024

Fixed the Unsupported version error when new admin tries to connect to old broker:

org.apache.kafka.common.errors.UnsupportedVersionException: Attempted to write a non-default timeoutMs at version 8

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@kamalcph kamalcph changed the title KAFKA-15859: Fixed the Unsupported version error when new admin tries to connect to old broker KAFKA-15859: Fixed the Unsupported version error when new admin connects to old broker Oct 3, 2024
@kamalcph kamalcph requested a review from lbradstreet October 3, 2024 09:59
@kamalcph
Copy link
Copy Markdown
Contributor Author

kamalcph commented Oct 3, 2024

The existing test testCheckEarliestLocalTimestampVersion and testCheckLatestTieredTimestampVersion in ListOffsetsRequestTest seems incorrect to me, it does not assert any behavior.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM!

@showuon
Copy link
Copy Markdown
Member

showuon commented Oct 3, 2024

The existing test testCheckEarliestLocalTimestampVersion and testCheckLatestTieredTimestampVersion in ListOffsetsRequestTest seems incorrect to me, it does not assert any behavior.

They assertThrows UnsupportedVersionException. What do you mean no assertion?

@kamalcph
Copy link
Copy Markdown
Contributor Author

kamalcph commented Oct 3, 2024

The existing test testCheckEarliestLocalTimestampVersion and testCheckLatestTieredTimestampVersion in ListOffsetsRequestTest seems incorrect to me, it does not assert any behavior.

They assertThrows UnsupportedVersionException. What do you mean no assertion?

The test throws unsupported version exception for all versions of LIST_OFFSETS request. If we update the testUnsupportedVersion method as below, then the test still succeeds:

private void testUnsupportedVersion(int version, long timestamp) {
        if (timestamp == EARLIEST_LOCAL_TIMESTAMP) {
            assertUnsupportedVersion(version);
        } else if (timestamp == LATEST_TIERED_TIMESTAMP) {
            assertUnsupportedVersion(version);
        }
    }

I don't understand the purpose of the test.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@kamalcph thanks for this patch

@Override
public ListOffsetsRequest build(short version) {
if (version >= 10) {
data.setTimeoutMs(timeoutMs);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please consider adding "ignorable": true to protocol file instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Updated the patch and tested out. It works as expected.

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Oct 3, 2024

The test throws unsupported version exception for all versions of LIST_OFFSETS request. If we update the testUnsupportedVersion method as below, then the test still succeeds:

you are right. in the #16876 we want to make sure the minVersion will be changed by the timestamp, those checks are covered by testListOffsetsRequestOldestVersion already. I open https://issues.apache.org/jira/browse/KAFKA-17693 to address it.

@showuon
Copy link
Copy Markdown
Member

showuon commented Oct 4, 2024

@kamalcph , you can try to merge this PR by yourselves now. Let us know if you encounter any problem.

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Oct 4, 2024

https://github.com/apache/kafka/blob/trunk/committer-tools/reviewers.py

Please consider leveraging 'reviewers.py' to generate the reviewer list 😃

@kamalcph kamalcph merged commit 5b3027d into apache:trunk Oct 4, 2024
@kamalcph kamalcph deleted the KAFKA-15859d branch October 4, 2024 08:19
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
…s to old broker (apache#17358)

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Luke Chen <showuon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants