Skip to content

Move nodeids to nosql package parent #2931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 31, 2025
Merged

Move nodeids to nosql package parent #2931

merged 2 commits into from
Oct 31, 2025

Conversation

@dimas-b
Copy link
Contributor

@dimas-b dimas-b commented Oct 28, 2025 •
edited
Loading

Following up on #2728 this change moves "nodeids" code to the org.apache.polaris.nosql.nodeids package.

@dimas-b dimas-b requested review from dennishuo and snazy October 28, 2025 23:34
@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Oct 28, 2025
@dimas-b dimas-b mentioned this pull request Oct 28, 2025
Merged
eric-maynard
eric-maynard previously approved these changes Oct 29, 2025
View reviewed changes
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Oct 29, 2025
@snazy
Copy link
Member

snazy commented Oct 29, 2025

Following up on #2728 this change moves "nodeids" code to the org.apache.polaris.nosql.nodeids package.

IMO it should be org.apache.polaris.persistence.nosql.nodeids to align with #1189

singhpk234
singhpk234 previously approved these changes Oct 29, 2025
View reviewed changes
@flyrain
Copy link
Contributor

flyrain commented Oct 29, 2025

IMO it should be org.apache.polaris.persistence.nosql.nodeids to align with #1189

+1 on it. We might change the other modules to be consistent in that case. They are with org.apache.polaris.nosql.* now.

@dimas-b
Move nodeids to nosql package parent
364de3c 
Following up on apache#2728 this change moves "nodeids" code to the
`org.apache.polaris.nosql.nodeids` package.
@dimas-b
review: persistence.nosql.nodeids
a370348
@dimas-b dimas-b dismissed stale reviews from singhpk234 and eric-maynard via a370348 October 29, 2025 17:32
@dimas-b
Copy link
Contributor Author

dimas-b commented Oct 29, 2025

Moved to org.apache.polaris.persistence.nosql.nodeids

@dimas-b
Copy link
Contributor Author

dimas-b commented Oct 29, 2025

@flyrain :

We might change the other modules to be consistent in that case

Do you have a concrete proposal for this? Thx!

@flyrain
Copy link
Contributor

flyrain commented Oct 29, 2025

We might change the other modules to be consistent in that case

Do you have a concrete proposal for this? Thx!

I was thinking of changing all org.apache.polaris.nosql.* to org.apache.polaris.persistence.nosql.* to be consistent.

@dimas-b
Copy link
Contributor Author

dimas-b commented Oct 29, 2025 •
edited
Loading

@flyrain :

I was thinking of changing all org.apache.polaris.nosql.* to org.apache.polaris.persistence.nosql.* to be consistent.

Would you mind opening a PR for that?

@dimas-b
Copy link
Contributor Author

dimas-b commented Oct 30, 2025 •
edited
Loading

@flyrain :

I was thinking of changing all org.apache.polaris.nosql.* to org.apache.polaris.persistence.nosql.* to be consistent.

Let's revisit this after the OPA PR #2680 merges. It already depends on some of those classes and I would not want to cause extra overhead in OPA work because that PR is already very old and has a lot of changes induced by reviews.

dennishuo
dennishuo approved these changes Oct 31, 2025
View reviewed changes
Copy link
Contributor

@dennishuo dennishuo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for following up on this!

@dimas-b dimas-b merged commit e08d5e2 into apache:main Oct 31, 2025
15 of 17 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Oct 31, 2025
@dimas-b dimas-b deleted the move-nodeids branch October 31, 2025 15:20
vchag pushed a commit to vchag/polaris that referenced this pull request Nov 5, 2025
@dimas-b @vchag
Move nodeids to nosql package parent (apache#2931)
ea7a2ec 
Following up on apache#2728 this change moves "nodeids" code to the
`org.apache.polaris.persistence.nosql.nodeids` package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@snazy snazy snazy approved these changes

@dennishuo dennishuo dennishuo approved these changes

@eric-maynard eric-maynard Awaiting requested review from eric-maynard

@singhpk234 singhpk234 Awaiting requested review from singhpk234

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dimas-b @snazy @flyrain @dennishuo @eric-maynard @singhpk234