Skip to content

MINOR: remove some specifying types in tool command#10329

Merged
chia7712 merged 6 commits intoapache:trunkfrom
wenbingshen:removeSpecifyType
Mar 17, 2021
Merged

MINOR: remove some specifying types in tool command#10329
chia7712 merged 6 commits intoapache:trunkfrom
wenbingshen:removeSpecifyType

Conversation

@wenbingshen
Copy link
Copy Markdown
Member

@wenbingshen wenbingshen commented Mar 16, 2021

As the title.I checked some of the command-line tools and removed some of the specified types.

Committer Checklist (excluded from commit message)

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

@wenbingshen
Copy link
Copy Markdown
Member Author

wenbingshen commented Mar 16, 2021

:) @dajac Can you help review this minor pr?Thanks.

chia7712
chia7712 reviewed Mar 16, 2021
View reviewed changes
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.

@wenbingshen thanks for your patch. As you are doing cleanup for AclCommand, could you remove all unnecessary type declaration from this class?

@wenbingshen
Copy link
Copy Markdown
Member Author

wenbingshen commented Mar 16, 2021

@wenbingshen thanks for your patch. As you are doing cleanup for AclCommand, could you remove all unnecessary type declaration from this class?

Thanks for your comments.I rechecked and performed the relevant unit tests.Please review again.

chia7712
chia7712 approved these changes Mar 17, 2021
View reviewed changes
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.

@wenbingshen thanks for your patch. LGTM

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Mar 17, 2021

@wenbingshen I believe those failed tests are unrelated to you patch. However, could you merge trunk to trigger QA again? We all love green QA :)

@wenbingshen
Copy link
Copy Markdown
Member Author

wenbingshen commented Mar 17, 2021

@wenbingshen I believe those failed tests are unrelated to you patch. However, could you merge trunk to trigger QA again? We all love green QA :)

Thank you for your review, I am very happy to do this. :)

dajac
dajac reviewed Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch. Left two minor stylistic comments.

Comment thread core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
reassignments: Map[TopicPartition, Seq[Int]]): Map[TopicPartition, Throwable] = {
val results: Map[TopicPartition, KafkaFuture[Void]] =
adminClient.alterPartitionReassignments(reassignments.map { case (part, replicas) =>
val results = adminClient.alterPartitionReassignments(reassignments.map { case (part, replicas) =>
Copy link
Copy Markdown
Member

@dajac dajac Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The indentation of the two lines below could be corrected now.

Comment thread core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala
: Map[TopicPartition, Throwable] = {
val results: Map[TopicPartition, KafkaFuture[Void]] =
adminClient.alterPartitionReassignments(reassignments.map {
val results = adminClient.alterPartitionReassignments(reassignments.map {
Copy link
Copy Markdown
Member

@dajac dajac Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@wenbingshen
Copy link
Copy Markdown
Member Author

wenbingshen commented Mar 17, 2021

Thanks for the patch. Left two minor stylistic comments.

Thank you for your comments, I have modified it, can you see see it again? Thank you. :)

dajac
dajac reviewed Mar 17, 2021
View reviewed changes
Comment thread core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala Outdated
(_, (None: Option[NewPartitionReassignment]).asJava)
}.toMap.asJava).values().asScala
val results = adminClient.alterPartitionReassignments(reassignments.map {
(_, (None: Option[NewPartitionReassignment]).asJava)
Copy link
Copy Markdown
Member

@dajac dajac Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are here, could we replace (None: Option[NewPartitionReassignment]).asJava by Optional.empty[NewPartitionReassignment]() or Option.empty[NewPartitionReassignment].asJava? Personally, I prefer using Optional directly here as we use Optional.of above.

Copy link
Copy Markdown
Member Author

@wenbingshen wenbingshen Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.I updated the code, please review it again, thank you.

dajac
dajac approved these changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending Jenkins. Thanks!

@wenbingshen
Copy link
Copy Markdown
Member Author

wenbingshen commented Mar 17, 2021

LGTM, pending Jenkins. Thanks!

Many thanks for your help!

@chia7712 chia7712 merged commit 0fb0440 into apache:trunk Mar 17, 2021
@ijuma ijuma mentioned this pull request Mar 17, 2021
Merged
3 tasks
ijuma added a commit to confluentinc/kafka that referenced this pull request Mar 17, 2021
@ijuma
CONFLUENT: Sync from apache/kafka/trunk (17 March 2021)
bea34ef 
Conflicts:
* Jenkinsfile: `install` -> `publishToMavenLocal`, drop ARM build and
other changes that don't make sense for Confluent's version of
`Jenkinsfile`.
* build.gradle: keep Confluent changes for automatic skipping signing
for specific version patterns (upstream only does it if the version ends
with `SNAPSHOT`).

Commits:
* apache-github/trunk: (59 commits)
  MINOR: Remove redundant allows in import-control.xml (apache#10339)
  MINOR: remove some specifying types in tool command (apache#10329)
  KAFKA-12455: Fix OffsetValidationTest.test_broker_rolling_bounce failure with Raft (apache#10322)
  MINOR: Add toString to various Kafka Metrics classes (apache#10330)
  KAFKA-12330; FetchSessionCache may cause starvation for partitions when FetchResponse is full (apache#10318)
  KAFKA-12427: Don't update connection idle time for muted connections (apache#10267)
  MINOR; Various code cleanups (apache#10319)
  HOTFIX: timeout issue in removeStreamThread() (apache#10321)
  revert stream logging level back to ERROR (apache#10320)
  KAFKA-12352: Make sure all rejoin group and reset state has a reason (apache#10232)
  KAFKA-10348: Share client channel between forwarding and auto creation manager (apache#10135)
  MINOR: Update year in NOTICE (apache#10308)
  KAFKA-12398: Fix flaky test `ConsumerBounceTest.testClose` (apache#10243)
  MINOR: Remove redundant inheritance from FilteringJmxReporter #onMetricRemoved (apache#10303)
  KAFKA-12462: proceed with task revocation in case of thread in PENDING_SHUTDOWN (apache#10311)
  KAFKA-12460; Do not allow raft truncation below high watermark (apache#10310)
  MINOR: Log project, gradle, java and scala versions at the start of the build (apache#10307)
  KAFKA-10357: Add missing repartition topic validation (apache#10305)
  MINOR: Improve error message in MirrorConnectorsIntegrationBaseTest (apache#10268)
  MINOR: Add missing unit tests for Mirror Connect (apache#10192)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dajac dajac dajac approved these changes

@chia7712 chia7712 chia7712 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wenbingshen @chia7712 @dajac