KAFKA-12183: Add the KIP-631 metadata record definitions #9876
Conversation
| { "name": "Id", "type": "int32", "versions": "0 ", | ||
| "about": "The broker ID to unfence." }, | ||
| { "name": "Epoch", "type": "int64", "versions": "0 ", | ||
| "about": "The epoch of the broker to unfence." } |
There was a problem hiding this comment.
In UnregisterBrokerRecord and RegisterBrokerRecord we call this BrokerId and BrokerEpoch receptively. Should we use the same names here for consistency?
| { "name": "Id", "type": "int32", "versions": "0 ", | ||
| "about": "The broker ID to fence. It will be removed from all ISRs." }, | ||
| { "name": "Epoch", "type": "int64", "versions": "0 ", | ||
| "about": "The epoch of the broker to fence." } |
There was a problem hiding this comment.
In UnregisterBrokerRecord and RegisterBrokerRecord we call this BrokerId and BrokerEpoch receptively. Should we use the same names here for consistency?
| "fields": [ | ||
| { "name": "Id", "type": "int32", "versions": "0 ", | ||
| "about": "The broker ID to fence. It will be removed from all ISRs." }, | ||
| { "name": "Epoch", "type": "int64", "versions": "0 ", |
There was a problem hiding this comment.
extra spaces before { "name":....
| // limitations under the License. | ||
|
|
||
| { | ||
| "apiKey": 14, |
There was a problem hiding this comment.
There is no message with apiKey 13.
There was a problem hiding this comment.
there is a message specified in the KIP. I'll add it.
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| @Timeout(value = 40) | ||
| public class MetadataParserTest { |
There was a problem hiding this comment.
Should we add a test that checks that the api keys for metadata records are consecutive numbers?
| * SHUTTING_DOWN | ||
| */ | ||
| @InterfaceStability.Evolving | ||
| public enum BrokerState { |
There was a problem hiding this comment.
Not that it does any harm, but is this class needed for this PR? Maybe these values are referenced by the metadata records?
There was a problem hiding this comment.
I was originally going to remove BrokerStates.scala and replace it with this BrokerState.java file. That was back when KIP-631 serialized broker states in the RPCs. But now that we're not doing that any more, there isn't really a strong reason to do this. So let's just skip it, I guess...
| compile libs.jacksonJDK8Datatypes | ||
| compileOnly libs.log4j | ||
| testCompile libs.junitJupiter | ||
| testCompile libs.hamcrest |
There was a problem hiding this comment.
Do we actually use hamcrest in this module? If not, we should remove this.
| testCompile libs.junitJupiter | ||
| testCompile libs.hamcrest | ||
| testCompile libs.slf4jlog4j | ||
| testCompile project(':clients').sourceSets.test.output // for org.apache.kafka.test.IntegrationTest |
There was a problem hiding this comment.
I think you don't need this if that comment is true.
There was a problem hiding this comment.
later PRs add IntegrationTest to the metadata module... simpler to just keep it in this PR
There was a problem hiding this comment.
This is not right, the mentioned annotation is only used for JUnit 4. I submitted a minor PR that fixes the comment at least: #9897
Add the metadata gradle module, which will contain the metadata record definitions, and other metadata-related broker-side code. Add MetadataParser, MetadataParseException, etc.
Add the metadata gradle module, which will contain the metadata record
definitions, and other metadata-related broker-side code.
Add MetadataParser, MetadataParseException, etc.