Skip to content

MINOR: Remove toStruct and fromStruct methods from generated protocol classes#9960

Merged
ijuma merged 5 commits intoapache:trunkfrom
ijuma:remove-to-struct-from-struct
Jan 25, 2021
Merged

MINOR: Remove toStruct and fromStruct methods from generated protocol classes#9960
ijuma merged 5 commits intoapache:trunkfrom
ijuma:remove-to-struct-from-struct

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Jan 25, 2021

Update few classes that were still using the removed methods (including
tests that are no longer required).

Committer Checklist (excluded from commit message)

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

…ocol classes

Update few classes that were still using the removed methods (including
tests that are no longer required).
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.

@ijuma Nice cleanup!

@@ -188,9 +185,7 @@ public KafkaPrincipal deserialize(byte[] bytes) {
throw new SerializationException("Invalid principal data version " + version);
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 we use HIGHEST_SUPPORTED_VERSION and LOWEST_SUPPORTED_VERSION to replace origin condition ("0" and DefaultPrincipalData.SCHEMAS.length) ?

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 point, addressed.

OffsetFetchResponse oldResponse = new OffsetFetchResponse(data, version);

if (version <= 1) {
assertFalse(struct.hasField(ERROR_CODE));
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.

the field "ERROR_CODE" is never used.


Deserializing Messages
----------------------
Message objects may be deserialized using the Message#read method. This method
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.

There is another invalid description.

You can also deserialize a message from a Struct by calling Message#fromStruct.
The Struct will not be modified.

@dengziming
Copy link
Copy Markdown
Member

This is nice, maybe we will also remove struct.writeTo and struct.read after #9641 #9766 and KAFKA-10863 is accomplished.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 25, 2021

@dengziming Looks like StickyAssignor also uses Struct still. Once all usages are removed, maybe we can delete Struct altogether instead of only some methods in it.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 25, 2021

Hmm, we may need to keep Struct as a type for Schema, however. So you may be right that we can only remove the serialization/deserialization methods.

@dengziming
Copy link
Copy Markdown
Member

dengziming commented Jan 25, 2021

Yes, #9766 is trying to replace Struct inStickyAssignor. I am not sure about removing Struct class, I will try to check it, but we can remove serialization/deserialization methods.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 25, 2021

JDK 11 build passed, JDK 8 and 15 had unrelated flaky failures.

@ijuma ijuma merged commit 411ac7d into apache:trunk Jan 25, 2021
ijuma added a commit to ijuma/kafka that referenced this pull request Jan 26, 2021
…e-allocations-lz4

* apache-github/trunk: (562 commits)
  MINOR: remove unused code from MessageTest (apache#9961)
  MINOR: Fix visibility of Log.{unflushedMessages, addSegment} methods (apache#9966)
  KAFKA-12229: Restore original class loader in integration tests using EmbeddedConnectCluster during shutdown  (apache#9942)
  KAFKA-12190: Fix setting of file permissions on non-POSIX filesystems (apache#9947)
  MINOR: Remove `toStruct` and `fromStruct` methods from generated protocol classes (apache#9960)
  MINOR: Fix typo in Utils#toPositive (apache#9943)
  MINOR: MessageUtil: remove some deadcode (apache#9931)
  MINOR: Update zstd-jni to 1.4.8-2 (apache#9957)
  MINOR: Revert assertion in MockProducerTest (apache#9956)
  MINOR: Optimize assertions in unit tests (apache#9955)
  MINOR: Tag `RaftEventSimulationTest` as `integration` and tweak it (apache#9925)
  MINOR: Update to Gradle 6.8.1 (apache#9953)
  MINOR: A few small group coordinator cleanups (apache#9952)
  MINOR: Upgrade ducktape to version 0.8.1  (apache#9933)
  MINOR: fix record time in test shouldWipeOutStandbyStateDirectoryIfCheckpointIsMissing (apache#9948)
  MINOR: Restore interrupt status when closing (apache#9863)
  KAFKA-10357: Extract setup of repartition topics from Streams partition assignor (apache#9848)
  KAFKA-12212; Bump Metadata API version to remove `ClusterAuthorizedOperations` fields (KIP-700) (apache#9945)
  MINOR: log 2min processing summary of StreamThread loop (apache#9941)
  MINOR: Drop enable.metadata.quorum config (apache#9934)
  ...
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.

3 participants