Skip to content

Fix verification of version probing#10943

Merged
cadonna merged 4 commits intoapache:trunkfrom
cadonna:fix_version_probing_system_test
Jul 12, 2021
Merged

Fix verification of version probing#10943
cadonna merged 4 commits intoapache:trunkfrom
cadonna:fix_version_probing_system_test

Conversation

@cadonna
Copy link
Copy Markdown
Member

@cadonna cadonna commented Jun 30, 2021

Committer Checklist (excluded from commit message)

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

@cadonna cadonna requested a review from ableegoldman June 30, 2021 10:54
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.

When did we bump the number?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here: #10609

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.

This manually number bumping is error-prone. Might need a better way to handle it in the future. What do you think?

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Jul 1, 2021

@showuon Agreed! All of us stumbled upon this. To be fair, usually we forget to adapt the test, but this time the author remembered to modify it, but missed to change one number. Such a situation could be changed by using variables. Let me have a stab at this.

@cadonna cadonna force-pushed the fix_version_probing_system_test branch from 39f5568 to 1742c09 Compare July 2, 2021 18:19
@ableegoldman
Copy link
Copy Markdown
Member

Wait, I'm confused. I definitely bumped the version number in this test in that PR you referenced (scroll down to the end of the changes, it's the last file). Did we for some reason bump the protocol version number again since then?

@ableegoldman
Copy link
Copy Markdown
Member

ableegoldman commented Jul 6, 2021

Oh nevermind, I see, I just missed bumping one instance of it in the message. In that case this LGTM

Copy link
Copy Markdown
Member

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up to use a variable and hopefully reduce how many times we have to fix this test going forward

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.

LGTM

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 add comments about why/when we need to bump this value? That can helps us from debugging this issue (again) in the future.

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.

Oh I actually added a comment in StreamsAssignmentProtocolVersions that describes everything you need to do when bumping the protocol number, and when you should/shouldn't bump it. It should cover everything but now the 1st instruction should be updated to say "bump the highest_version variable" instead of saying to update these messages directly @cadonna

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea! I updated the instructions.

@cadonna cadonna force-pushed the fix_version_probing_system_test branch from 1742c09 to 8dc447f Compare July 12, 2021 09:41
@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Jul 12, 2021

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Jul 12, 2021

Failed tests are unrelated and known to be flaky.

@cadonna cadonna merged commit 332db13 into apache:trunk Jul 12, 2021
cadonna added a commit to cadonna/kafka that referenced this pull request Jul 12, 2021
Fixes and improves version probing in system test test_version_probing_upgrade().
cadonna added a commit that referenced this pull request Jul 12, 2021
Fixes and improves version probing in system test test_version_probing_upgrade().
@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Jul 12, 2021

@kkonstantine Cherry-picked to 3.0 since it fixes a consistently failing system test also there.

@cadonna
Copy link
Copy Markdown
Member Author

cadonna commented Jul 12, 2021

xdgrulez pushed a commit to xdgrulez/kafka that referenced this pull request Dec 22, 2021
Fixes and improves version probing in system test test_version_probing_upgrade().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants