MINOR: improve the Kafka RPC code generator#7340
Conversation
soondenana
left a comment
soondenana
left a comment
There was a problem hiding this comment.
Thanks for taking care of this. Liked Version.subtract abstraction.
Left a bunch of nits. LGTM.
cmccabe
force-pushed
the
generator_improvements
branch
from
1b4c175 to
daa85c2
Compare
jsancio
left a comment
jsancio
left a comment
There was a problem hiding this comment.
Thanks for the PR. Left some comments.
There was a problem hiding this comment.
Why null? Should we just throw illegal argument or something?
There was a problem hiding this comment.
It's not necessarily illegal -- we sometimes want to handle this case. I could create a special exception for this, but returning null seemed simpler.
There was a problem hiding this comment.
I think I'd prefer something other than null here. It looks like Versions are copied around to a few places where we would need to worry about NPEs. Optional feels overkill, so maybe just another special Versions instance like INVALID or something?
There was a problem hiding this comment.
There isn't a Versions#INVALID and I don't think we should create one. All Versions objects currently represent a version range conceptually, and I think that's a good property to keep. There is a Versions#NONE, but that isn't what we want here (it is the empty range).
I think the choices are basically returning null, returning None, or throwing an exception.
If we do choose null, we don't actually have to check for it since Java already does everywhere. I can see the argument for returning Some / None, though, since it makes it clearer that this function may return nothing. In practice, most of the time when we call subtract, we know that it will return a valid result, so I don't bother checking. Java will throw an NPE if a precondition was violated somewhere.
I did consider implementing collections of ranges so that this function could be defined over all inputs. But in practice, this massively increased the complexity of the Versions code for no benefit in practice.
There was a problem hiding this comment.
Understood. Thanks for the clarification
Move the generator checkstyle suppressions to a special section, rather than mixing them in with the other sections. For generated code, do not complain about variable names or cyclic complexity. FieldType.java: remove isInteger since it isn't used anywhere. This way, we don't have to decide whether a UUID is an integer or not (there are arguments for both choices). Add FieldType#serializationIsDifferentInFlexibleVersions and FieldType#isVariableLength. HeaderGenerator: add the ability to generate static imports. Add IsNullConditional, VersionConditional, and ClauseGenerator as easier ways of generating "if" statements. Add Versions#subtract, and a unit test for it. Add CodeBufferTest to test the existing CodeBuffer class.
Add more JavaDoc to allowMembershipCheckAlwaysFalse and alwaysEmitBlockScope.
cmccabe
force-pushed
the
generator_improvements
branch
from
daa85c2 to
7cb8abd
Compare
|
Looks like a Jenkins environment issue in the latest tests. I will re-trigger the tests. |
|
retest this please |
Conflicts: * .gitignore: addition of clients/src/generated-test was near local additions for support-metrics. * checkstyle/suppressions.xml: upstream refactoring of exclusions for generator were near the local changes for support-metrics. * gradle.properties: scala version bump caused a minor conflict due to the kafka version change locally. gradle/dependencies.gradle: bcpkix version bump was near avro additions in the local version. * apache-github/trunk: (49 commits) KAFKA-8471: Replace control requests/responses with automated protocol (apache#7353) MINOR: Don't generate unnecessary strings for debug logging in FetchSessionHandler (apache#7394) MINOR:fixed typo and removed outdated varilable name (apache#7402) KAFKA-8934: Create version file during build for Streams (apache#7397) KAFKA-8319: Make KafkaStreamsTest a non-integration test class (apache#7382) KAFKA-6883: Add toUpperCase support to sasl.kerberos.principal.to.local rule (KIP-309) KAFKA-8907; Return topic configs in CreateTopics response (KIP-525) (apache#7380) MINOR: Address review comments for KIP-504 authorizer changes (apache#7379) MINOR: add versioning to request and response headers (apache#7372) KAFKA-7273: Extend Connect Converter to support headers (apache#6362) MINOR: improve the Kafka RPC code generator (apache#7340) MINOR: Improve the org.apache.kafka.common.protocol code (apache#7344) KAFKA-8880: Docs on upgrade-guide (apache#7385) KAFKA-8179: do not suspend standby tasks during rebalance (apache#7321) KAFKA-8580: Compute RocksDB metrics (apache#7263) KAFKA-8880: Add overloaded function of Consumer.committed (apache#7304) HOTFIX: fix Kafka Streams upgrade note for broker backward compatibility (apache#7363) KAFKA-8848; Update system tests to use new AclAuthorizer (apache#7374) MINOR: remove unnecessary null check (apache#7299) KAFKA-6958: Overload methods for group and windowed stream to allow to name operation name using the new Named class (apache#6413) ...
4 participants
Move the generator checkstyle suppressions to a special section, rather than mixing them in with the other sections. For generated code, do not complain about variable names or cyclic complexity.
Remove FieldType#isInteger. This way, we don't have to decide whether a UUID is an integer or not (there are arguments for both choices). Add FieldType#serializationIsDifferentInFlexibleVersions and FieldType#isVariableLength.
HeaderGenerator: add the ability to generate static imports. Add
IsNullConditional, VersionConditional, and ClauseGenerator as easier ways of
generating "if" statements.
Add Versions#subtract, and a unit test for it.
Add CodeBufferTest to test the existing CodeBuffer class.