KAFKA-18134: Disallow group upgrades when custom assignors are used #18046
Conversation
Disallow upgrades from classic groups to consumer groups when any member's assignment has non-empty userData.
dajac
added
KIP-848
There was a problem hiding this comment.
@squah-confluent Thanks for the patch! I left a few small comments.
| throw new GroupIdNotFoundException("Cannot upgrade the classic group " + classicGroup.groupId() + | ||
| " to consumer group because a custom assignor is in use."); |
There was a problem hiding this comment.
Completely unrelated but I wonder if we always provide such good error message when the upgrade is not possible. Do you mind checking?
There was a problem hiding this comment.
nit: I wonder if we could extend the error message a bit more. We actually support upgrading while using a custom assignor but only if it does not have user data associated to the assignment. We could perhaps explain why they should do in this case (e.g. move to a default assignor and then upgrade?)?
There was a problem hiding this comment.
Completely unrelated but I wonder if we always provide such good error message when the upgrade is not possible. Do you mind checking?
I had a look and there are three reasons in validateOnlineUpgrade that we convert to "Group %s is not a consumer group":
- Cannot upgrade classic group {} to consumer group because the online upgrade is disabled.
- Cannot upgrade classic group {} to consumer group because the group does not use the consumer embedded protocol.
- Cannot upgrade classic group {} to consumer group because the group size exceeds the consumer group maximum size.
Apart from that and SchemaException when subscriptions or metadata are malformed, I can't see any other error paths directly related to the upgrade.
There was a problem hiding this comment.
Thanks for the review! I've reworked the error message. Let me know what you think.
There was a problem hiding this comment.
I had a look and there are three reasons in validateOnlineUpgrade that we convert to "Group %s is not a consumer group":
I wonder whether we should extend the error message that we provide to the user. "not a consumer group" does not say much. What do you think?
There was a problem hiding this comment.
I think it's a good idea. I've updated validateOnlineUpgrade to throw an exception with the appropriate message instead. And tweaked some of the error wording to be consistent with the rest.
| ); | ||
| if (assignment.userData() != null && | ||
| assignment.userData().hasRemaining()) { |
There was a problem hiding this comment.
nit: We could bring it back on the previous line. There is enough space.
| assertFalse(expectUpgrade); | ||
| assertEquals("Cannot upgrade the classic group group-id to consumer group because a custom assignor is in use.", ex.getMessage()); |
There was a problem hiding this comment.
nit: It is usually better to use assertThrows. Would it be possible to use it in this case?
There was a problem hiding this comment.
I've updated the test. Let me know what you think.
| " to consumer group: " + e.getMessage() + ".", e); | ||
|
|
||
| throw new GroupIdNotFoundException("Cannot upgrade the classic group " + classicGroup.groupId() + | ||
| " to consumer group because a custom assignor with userData is in use. " + |
There was a problem hiding this comment.
I wonder if we could make userData clearer. Most of the users won't understand it. Should we perhaps say ... because an unsupported custom assignor is in use. Please refer to the document or switch to ....
There was a problem hiding this comment.
To clarify, which document should I direct users to?
There was a problem hiding this comment.
Sorry, I meant documentation, which implies the Kafka doc.
There was a problem hiding this comment.
Reworded the error to Cannot upgrade classic group %s to consumer group because an unsupported custom assignor is in use. Please refer to the documentation or switch to a default assignor before re-attempting the upgrade.
| "Switch to a default assignor before re-attempting the upgrade.", ex.getMessage()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder whether we could add an integration test in ConsumerProtocolMigrationTest. Is it worth?
There was a problem hiding this comment.
I didn't know we had integration tests for group upgrades. This is definitely worth an integration test. I've gone ahead and added one. Let me know what you think.
dajac
deleted the
squah-disallow-classic-group-upgrades-with-assignment-userdata
branch
…pache#18046) Disallow upgrades from classic groups to consumer groups when any member's assignment has non-empty userData. Reviewers: David Jacot <[email protected]>
Disallow upgrades from classic groups to consumer groups when any
member's assignment has non-empty userData.
Committer Checklist (excluded from commit message)