Restructure the directory and package name for persistence modules #1724
Conversation
flyrain
requested review from
HonahX,
MonkeyCanCode,
RussellSpitzer,
adutra,
ajantha-bhat,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
jackye1995,
jbonofre,
pingtimeout,
singhpk234,
snazy,
takidau and
vvcephei
as code owners
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
The file moves look reasonable to me, but java package renames look a bit inconsistent across the affected code (specific comment below).
However, please be careful with PR descriptions "remove extension directory" is a very strong statement :)
I'd appreciate a dev ML discussion about the general direction of the repository layout changes before PRs that move stuff are submitted for review.
There was a problem hiding this comment.
Why do we move to a different package in other classes, but keep package org.apache.polaris.extension.persistence.... here?
There was a problem hiding this comment.
This was intended to not change the package name for the module eclipse link. Two reasons:
- It's deprecated, will be removed later. We should be fine with the temporary name inconsistency.
- It has been used by certain OSS users already. Changing the package name may break downstream, esp. for users who adopted EclipseLink with some customizations.
There was a problem hiding this comment.
Fair enough... although, I guess the deeper implication is that we probably need to documents at some point what modules/classes downstream projects should expect to be "stable" what what is "subject to change without notice".
Without defining that across the whole Polaris codebase if would be pretty much impossible to have a common understanding among committers what to flag as compatibility issues in reviews and what is ok 🤔
There was a problem hiding this comment.
That's a good point. We need to be careful about REST API compatibility for sure. But I don't worry too much about the Java APIs, as Polaris is a server rather than a lib or libs. I think we still need to make sure certain level of compatibilities, like public methods in the core module, esp. things like event listener APIs, etc. Feel free to propose something.
There was a problem hiding this comment.
I think we still need at least some degree of guarantee for Java APIs as well. There are a few vendors integrating Polaris in their own offerings, and a change in public method in any service module (not only core) may create problems downstream. Configuration changes are another area where some guarantees should exist.
That said, I also think that until we get to 1.0.0, any changes should be allowed as long as they are clearly communicated. So for now I'm OK with the proposed refactoring here.
There was a problem hiding this comment.
Good point, @adutra! The API interface scope worths separated discussion in the dev ML. Let's move on with a dedicated discussion.
flyrain
changed the title
github-project-automation
bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
github-project-automation
bot
moved this from Done
to PRs In Progress
in Basic Kanban Board
flyrain
force-pushed
the
remove-extension-prefix
branch
from
bc52dec to
8f71e59
Compare
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
looks good to me !
DISCUSS thread : https://lists.apache.org/thread/4yc7ol2oo8n0hpq0tfgyvjo1nswfy5oh
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
* Fix regression test docker setup for purge (apache#1768) * Use canonical catalog property names in tests (apache#1766) * In `PolarisPolicyServiceIntegrationTest` * In `PolarisRestCatalogIntegrationTest` Following up to apache#1557 * Unblock test `createViewWithCustomMetadataLocation` (apache#1320) * Add test for invalid custom metadata location * Add missing properties during table/view creation * main: Update docker.io/prom/prometheus Docker tag to v3.4.1 (apache#1767) * Testing: silence a bunch of harmless test warnings (apache#1773) * `OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended` * Hibernate Validator cannot instrument static methods (`Hibernate Validator does not support constraints on static methods yet. ...`) * ForkJoinPool test lifecycle warning * Couple of split-package warnings * Add unit test for legacy config lookup (apache#1774) Following up on apache#1766 * Handle RequestScoped instance injection gracefully for DefaultConfigurationStore (apache#1758) Although we do a check of !realmContextInstance.isUnsatisfied() it only checks if there is matching bean, however, it doesn't check if the context is active or not. Similarly isResolvable also don't check if the scope is active. Therefore if getConfiguration is called when there is no active scope, it will get Exception like ContextNotActiveException. This actually fails the TaskExecutor because it runs in a separate thread. In order to fix the problem, we introduces a new getConfiguration function to handle the background tasks, and also use isResolvable instead of isUnsatisfied to handle ambiguities. * Restructure the directory and package name for persistence modules (apache#1724) * Re-add missing parameters to create_table python API (apache#1778) It looks like this method lost the `prefix` and `namespace` parameters in apache#1347. They're re-introduced to the spec here, and then I've run: ``` redocly bundle spec/polaris-catalog-service.yaml -o spec/generated/bundled-polaris-catalog-service.yaml ./gradlew regeneratePythonClient ``` Then, some manual reverts: ``` alias gitrevert='git checkout upstream/main --' gitrevert client/python/.github/workflows/python.yml gitrevert client/python/.gitlab-ci.yml gitrevert client/python/pyproject.toml ``` I still hope to automate this process as part of CI soon; see apache#1675 * Replace getConfiguration usage with PolarisCallContext to use RealmContext (PART 1) (apache#1780) * JDBC: Fix getting started config (apache#1781) Fix typo in the JDBC config for getting started, the config should be max_duration_in_ms instead of max_delay_in_ms * main: Pin dependencies (apache#1701) * main: Update dependency pytest to v8 (apache#1710) * main: Update dependency boto3 to v1.38.28 (apache#1777) * Run renovatebot only on the main branch (apache#1786) * INFO: last merged commit a827d26 --------- Co-authored-by: gh-yzou <167037035+gh-yzou@users.noreply.github.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Liam Bao <liam.zw.bao@gmail.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Yufei Gu <yufei@apache.org> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com> Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com> Co-authored-by: JB Onofré <jbonofre@apache.org>
4 participants
A directory refactor + package name refactor. No logic change.