Skip to content

MINOR: refactor result string#12389

Merged
showuon merged 1 commit intoapache:trunkfrom
pch8388:minor-result-string
Jul 11, 2022
Merged

MINOR: refactor result string#12389
showuon merged 1 commit intoapache:trunkfrom
pch8388:minor-result-string

Conversation

@pch8388
Copy link
Contributor

@pch8388 pch8388 commented Jul 7, 2022

Removes duplicate result strings and avoids mistakes when changing the string format

Committer Checklist (excluded from commit message)

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

@pch8388
Copy link
Contributor Author

pch8388 commented Jul 11, 2022

@ijuma would you please review this?

showuon
showuon approved these changes Jul 11, 2022
View reviewed changes
Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM!

@showuon
Copy link
Member

showuon commented Jul 11, 2022

Failed tests are unrelated:

    Build / PowerPC / Run PowerPC Build / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testFencedLeaderRecovery
    Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testFencedLeaderRecovery

@showuon showuon merged commit 23c92ce into apache:trunk Jul 11, 2022
@pch8388
Copy link
Contributor Author

pch8388 commented Jul 11, 2022

Thanks for the review and merge!

@pch8388 pch8388 deleted the minor-result-string branch July 11, 2022 06:44
@ijuma
Copy link
Member

ijuma commented Jul 11, 2022

Thanks for the PR. Can you please include a unit test for this fix? Also, String.format performs worse than string concatenation. Seems ok here, but worth keeping in mind for areas where performance is important.

@ijuma
Copy link
Member

ijuma commented Jul 11, 2022 •
edited
Loading

@showuon Whenever reviewing PRs for fixes, we should generally include at least a unit test. There needs to be a strong reason to merge a fix without any test changes/additions.

@pch8388 pch8388 restored the minor-result-string branch July 11, 2022 07:34
@pch8388
Copy link
Contributor Author

pch8388 commented Jul 11, 2022

Thanks for the review.
How do I add changes to a merged PR?
Need to open a new PR?

@showuon
Copy link
Member

showuon commented Jul 11, 2022 •
edited
Loading

@ijuma , thanks for the reminder. But I've checked and confirmed there is already a unit test covered this change: ConfigDefTest#testNiceMemoryUnits. I should have mentioned it in the review comments. I'll do that next time to make it clear. Thanks.

@ijuma
Copy link
Member

ijuma commented Jul 11, 2022

The PR description says "Fix...". Are we saying it's not a fix, it's simply a refactoring? We should make it clear if so.

@pch8388
Copy link
Contributor Author

pch8388 commented Jul 11, 2022

That's my mistake.
I should say it's a simple refactoring.
I'll edit the PR title.

@showuon
Copy link
Member

showuon commented Jul 11, 2022

Good point. Updated.

@showuon showuon changed the title MINOR: Fix result string MINOR: refactor result string Jul 11, 2022
@pch8388
Copy link
Contributor Author

pch8388 commented Jul 11, 2022

Thanks for the good point

ijuma added a commit to confluentinc/kafka that referenced this pull request Aug 3, 2022
@ijuma
Merge up to the Apache Kafka 3.3 branching point
affe8cd 
> $ git merge-base apache-github/3.3 apache-github/trunk
> 23c92ce

> $ git show 23c92ce
> commit 23c92ce
> Author: SC <pch838811@gmail.com>
> Date:   Mon Jul 11 11:36:56 2022 +0900
>
>    MINOR: Use String#format for niceMemoryUnits result (apache#12389)
>
>    Reviewers: Luke Chen <showuon@gmail.com>, Divij Vaidya <diviv@amazon.com>

* commit '23c92ce79366e86ca719e5e51c550c27324acd83':
  MINOR: Use String#format for niceMemoryUnits result (apache#12389)
  KAFKA-14055; Txn markers should not be removed by matching records in the offset map (apache#12390)
  KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection (apache#12381)
  KAFKA-13996: log.cleaner.io.max.bytes.per.second can be changed dynamically (apache#12296)
  KAFKA-13983: Fail the creation with "/" in resource name in zk ACL (apache#12359)
  KAFKA-12943: update aggregating documentation (apache#12091)
  KAFKA-13846: Follow up PR to address review comments (apache#12297)
This was referenced Aug 3, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@divijvaidya divijvaidya divijvaidya approved these changes

@showuon showuon showuon 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.

4 participants

@pch8388 @showuon @ijuma @divijvaidya