Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Broker class based defaults #7631
Broker class based defaults #7631
Changes from 40 commits
d7ee0ca
66dfc36
662c214
3c11b02
8145609
0f3b4a2
eca570c
5f61000
b8a649e
73eb74d
03f8cab
6395453
15fa1c4
2480d40
a70189f
76592bb
9b5abf3
4a2da5f
3813fa6
d0a1660
51c88f7
c678829
28c30e6
d37c5e3
f602457
b5a6863
37c637a
5f6d615
ef6c1c5
4106cbc
77779fd
fcf9916
606578c
3bcdf2f
78c73e2
aef75f5
5333a8a
602427d
8c1e77f
2f36466
d9df59f
96ae24c
b928021
27eb33c
fe6d9dc
b4f7060
7a7260f
0e62b00
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 1 in pkg/apis/config/defaults.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we check before, if
d.ClusterDefaultConfig.BrokerClass == brokerClassName
and if this is the case returnd.ClusterDefaultConfig.BrokerConfig
, as you wrote in https://docs.google.com/document/d/1RKij-DYPmcbCTHF26hkXdrtWHqrksA-Fo5qYLRP7xVA/editOtherwise
d.ClusterDefaultConfig.BrokerConfig
does not take preference overd.ClusterDefaultConfig.BrokerClasses[]
(what I meant in #7631 (comment))There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic here is correct, prob I didn't explain clearly in the Doc, sorry about that! @creydr
if
d.ClusterDefaultConfig.BrokerClass == brokerClassName
, then we will directly used.ClusterDefaultConfig.BrokerConfig
. And this brokerClass shouldn't appear ind.ClusterDefaultConfig.BrokerClasses
.When I was referring to:
It means the brokerConfig for
d.ClusterDefaultConfig.BrokerClass
will not be included in thed.ClusterDefaultConfig.BrokerClasses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to return an error here instead? At this point we know there is no config for the requested broker class, so we might get weird behaviour for brokers with a random config that doesn't match their class. @pierDipi @creydr any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @Leo6Leo - wdyt here? From the previous comment, my intuition is that this should return an error, but I'm not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the requested
brokerClass
lacks a specific configuration, the system should fallback to using the default brokerClass and its configuration, rather than throwing an error. This behavior aligns with the purpose of having a default configuration in place in my opinion. But returning the error also makes sense to me.