Skip to content

MINOR: Rename log dir UUIDs#14517

Merged
dengziming merged 3 commits into
apache:trunkfrom
soarez:dir-uuids-rename
Oct 30, 2023
Merged

MINOR: Rename log dir UUIDs#14517
dengziming merged 3 commits into
apache:trunkfrom
soarez:dir-uuids-rename

Conversation

@soarez

@soarez soarez commented Oct 10, 2023

Copy link
Copy Markdown
Member

After a late discussion in the voting thread for KIP-858 we decided to improve the names for the designated reserved log directory UUID values.

Committer Checklist (excluded from commit message)

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

After a late discussion in the voting thread for KIP-858 we
decided to improve the names for the designated reserved
log directory UUID values.
@soarez
soarez marked this pull request as ready for review October 10, 2023 04:28

@clolov clolov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For easy discoverability, this is the link to the discussion - https://lists.apache.org/thread/38sbj42ssoksjq2p488vdk1992dw3j1t. The change makes sense to me, thank you for persevering with this KIP!

* A UUID that is used to identify new or unknown dir assignments.
*/
public static final Uuid UNKNOWN_DIR = ZERO_UUID;
public static final Uuid UNASSIGNED_DIR = ZERO_UUID;

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 Uuid class should not know about any of this. We should move this logic to the module where it makes sense.

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.

Ok. Directory IDs will be used from metadata, core, storage and tools. I've moved them to a separate class, but I'm not sure which module would be a better fit than clients for this. Do you have a suggestion?

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.

server-common probably.

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.

Thanks for the suggestion. I've moved the class to that module.

*/
public static Uuid random() {
Uuid uuid = Uuid.randomUuid();
while (RESERVED.contains(uuid) || uuid.toString().startsWith("-")) {

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 don't need uuid.toString().startsWith("-") here, right?

@dengziming dengziming 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, I think it's OK to check directory it twice.

@soarez soarez mentioned this pull request Oct 29, 2023
3 tasks
@dengziming
dengziming merged commit 9dbee59 into apache:trunk Oct 30, 2023
@soarez

soarez commented Oct 30, 2023

Copy link
Copy Markdown
Member Author

Thank you for reviewing and merging @dengziming 🙏

yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
After a late discussion in the voting thread for KIP-858 we
decided to improve the names for the designated reserved
log directory UUID values.

Reviewers: Christo Lolov <lolovc@amazon.com>, Ismael Juma <ismael@juma.me.uk>,  Ziming Deng <dengziming1993@gmail.com>.
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
After a late discussion in the voting thread for KIP-858 we
decided to improve the names for the designated reserved
log directory UUID values.

Reviewers: Christo Lolov <lolovc@amazon.com>, Ismael Juma <ismael@juma.me.uk>,  Ziming Deng <dengziming1993@gmail.com>.
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.

4 participants