Skip to content

MINOR: refactor(storage): topic-based RLMM consumer-manager/task related improvements#14045

Merged
satishd merged 5 commits into
apache:trunkfrom
jeqo:jeqo/refactor-tbrlmm
Jul 22, 2023
Merged

MINOR: refactor(storage): topic-based RLMM consumer-manager/task related improvements#14045
satishd merged 5 commits into
apache:trunkfrom
jeqo:jeqo/refactor-tbrlmm

Conversation

@jeqo

@jeqo jeqo commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

Moving refactoring changes from #14012 into a separate PR.

Committer Checklist (excluded from commit message)

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

@showuon showuon left a comment

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.

@jeqo , thanks for the refactor. Left some comments. BTW, could you prefix the PR title with MINOR: refactor(storage): topic-based RLMM .... Thanks.

Comment on lines 90 to 100

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.

This change should be reverted since the parameter is not updated.

Comment on lines 185 to 187

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.

We should pass recordMetadata only, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Of course, missed this one -- pushed fixes.

@jeqo jeqo changed the title refactor(storage): topic-based RLMM consumer-manager/task related improvements MINOR: refactor(storage): topic-based RLMM consumer-manager/task related improvements Jul 20, 2023
@jeqo
jeqo force-pushed the jeqo/refactor-tbrlmm branch 2 times, most recently from 00e0848 to d32810f Compare July 20, 2023 07:58

@satishd satishd left a comment

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.

Thanks @jeqo for the PR. Left a minor comment.

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.

Can we return an immutable Set of assignedMetaPartitions instead of topic-partitions as the topic names are repetitive? This is currently being passed to build a string of partitions in an Exception message here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, applying suggestion

@showuon showuon left a comment

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.

LGTM!

@satishd satishd left a comment

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.

Thanks @jeqo for addressing the review comments. LGTM.

@showuon

showuon commented Jul 21, 2023

Copy link
Copy Markdown
Member

Rerunning CI build: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14045/6/

@divijvaidya divijvaidya added the tiered-storage Related to the Tiered Storage feature label Jul 21, 2023
@showuon

showuon commented Jul 21, 2023

Copy link
Copy Markdown
Member

@jeqo , there's a compilation error, could you take a look?
https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-14045/6/pipeline

Not sure if it is related to this fix: #14065 . If so, please try rebase to the latest trunk and see if it fixes it. Thanks.

@jeqo
jeqo force-pushed the jeqo/refactor-tbrlmm branch from f7a78e6 to 64e1496 Compare July 21, 2023 10:02
@jeqo

jeqo commented Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

It seems to, force-pushed now.

@satishd

satishd commented Jul 22, 2023

Copy link
Copy Markdown
Member

A few failed tests are observed on the Jenkins build jobs but they do not seem to be related, merging it to trunk.

@satishd
satishd merged commit cc4e699 into apache:trunk Jul 22, 2023
@jeqo
jeqo deleted the jeqo/refactor-tbrlmm branch July 24, 2023 05:27
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
… consumer-manager/task (apache#14045)

Improved logging and docs on consumer manager/task call paths.

Reviewers: Luke Chen <showuon@gmail.com>, Satish Duggana <satishd@apache.org>
jeqo added a commit to aiven/kafka that referenced this pull request Aug 15, 2023
… consumer-manager/task (apache#14045)

Improved logging and docs on consumer manager/task call paths.

Reviewers: Luke Chen <showuon@gmail.com>, Satish Duggana <satishd@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tiered-storage Related to the Tiered Storage feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants