Skip to content

Isolate Persistence objects in different threads. #1166

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

Merged
merged 5 commits into from
Mar 14, 2025
Merged

Isolate Persistence objects in different threads. #1166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a342f9b
Isolate Persistence objects in different threads.
dimas-b Mar 12, 2025
6c515a1
Implement copyOf:
dimas-b Mar 13, 2025
ba9ca8f
review: remove unnecessary debug log
dimas-b Mar 14, 2025
a38990d
review: revert copyOf impl
dimas-b Mar 14, 2025
48b837c
review: rename to detach()
dimas-b Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public PolarisCallContext(

public static PolarisCallContext copyOf(PolarisCallContext original) {
return new PolarisCallContext(
original.getMetaStore(),
original.getMetaStore().detach(),
original.getDiagServices(),
original.getConfigurationStore(),
original.getClock());
Expand Down
11 changes: 11 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/BasePersistence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,4 +392,15 @@ boolean hasChildren(
@Nullable PolarisEntityType optionalEntityType,
long catalogId,
long parentId);

/**
* Performs operations necessary to isolate the state of {@code this} {@link BasePersistence}
* instance from the state of the returned instance as far as multithreaded usage is concerned. If
* the implementation has state that is not supposed to be accessed or modified by multiple
* threads, it may return a copy from this method. If the implementation is thread-safe, it may
* return {@code this}.
*/
default BasePersistence detach() {
return this;
Copy link
Contributor

Choose a reason for hiding this comment

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

The name copyOf is confusing when we allow it to return itself. I'd recommend to either give a name with wider scope or we don't provide a default impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT about disengage()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just always return a new instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

For that matter, why not implement Cloneable?

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe remove the default impl and let implementations return the copy - as it's done for all the other objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cloneable is too deeply rooted in the JRE... I prefer simple copy via explicit constructor. Will update presently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

Choose a reason for hiding this comment

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

If the base idea behind this PR is to actually provide a deep clone of PolarisCallContext, I'd vote to rename all copyOf methods to something like deepClone or deepCopy. But since that's largely a matter of taste, I'm fine with copyOf as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specific change is needed for the upcoming NoSQL persistence.

I'm open to call context refactoring too, but I guess we may want to handle thread locals first?.. Then, perhaps deep clone will not be necessary 🤔 Cf. #463

}
}