-
Notifications
You must be signed in to change notification settings - Fork 357
[DRAFT] [DO NOT REVIEW] Introduce CachedSupplier for BasePersistence objects #1765
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.polaris.core.utils; | ||
|
|
||
| import java.util.function.Supplier; | ||
|
|
||
| public class CachedSupplier<T> implements Supplier<T> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC this functionality already exists in Guava.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL - I believe you're talking about |
||
| private final Supplier<T> delegate; | ||
| private T value; | ||
| private boolean initialized = false; | ||
|
|
||
| public CachedSupplier(Supplier<T> delegate) { | ||
| this.delegate = delegate; | ||
| } | ||
|
|
||
| @Override | ||
| public synchronized T get() { | ||
| if (!initialized) { | ||
| value = delegate.get(); | ||
| initialized = true; | ||
| } | ||
| return value; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.polaris.core.utils; | ||
|
|
||
| import java.util.function.Supplier; | ||
| import org.apache.polaris.core.context.RealmContext; | ||
| import org.assertj.core.api.Assertions; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class CachedSupplierTest { | ||
| private String realmName = "test"; | ||
| private int timesCalled = 0; | ||
| private final RealmContext realmContext = | ||
| new RealmContext() { | ||
| @Override | ||
| public String getRealmIdentifier() { | ||
| if (++timesCalled == 1) { | ||
| return realmName; | ||
| } | ||
| throw new IllegalStateException(); | ||
| } | ||
| }; | ||
|
|
||
| private static class ContainerRealmIdentifier { | ||
| private String realmIdentifier; | ||
|
|
||
| public ContainerRealmIdentifier(RealmContext realmContext) { | ||
| this.realmIdentifier = realmContext.getRealmIdentifier(); | ||
| } | ||
|
|
||
| public String getRealmIdentifier() { | ||
| return realmIdentifier; | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testCachedSupplier() { | ||
| Supplier<ContainerRealmIdentifier> realmIdentifierSupplier = | ||
| () -> new ContainerRealmIdentifier(realmContext); | ||
| Assertions.assertThat(realmName.equals(realmIdentifierSupplier.get().getRealmIdentifier())) | ||
| .isTrue(); // This will work | ||
| Assertions.assertThatThrownBy(() -> realmIdentifierSupplier.get().getRealmIdentifier()) | ||
| .isInstanceOf(IllegalStateException.class); | ||
|
|
||
| timesCalled = 0; | ||
| CachedSupplier<ContainerRealmIdentifier> cachedSupplier = | ||
| new CachedSupplier<>(() -> new ContainerRealmIdentifier(realmContext)); | ||
| Assertions.assertThat(realmName.equals(cachedSupplier.get().getRealmIdentifier())) | ||
| .isTrue(); // This will work | ||
| Assertions.assertThat(realmName.equals(cachedSupplier.get().getRealmIdentifier())) | ||
| .isTrue(); // This will work | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this change conforms with the behavior intended by the
sessionSupplierMap.Currently, each call to the supplier yields a new instance - this PR updates the behavior to provide a lazily initialized
Persistenceinstance per realm-ID.Maybe @collado-mike or @dennishuo could chime in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. This makes me wonder - would my original idea of materializing the
RealmContextprior to the creation of the Supplier also become an issue? For instance, would it be possible that theRealmContextis also be computed lazily in some instances?Let me follow up with @collado-mike and @dennishuo on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, each instance of the session was intended to be scoped to a single request. Though, it seems now that all the current implementations are stateless, but the
TransactionalPersistenceinterface methods kind of imply a stateful implementation - e.g.,lookupEntityInCurrentTxnassumes that there is a current transaction that has already been started and will be committed at some point.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, so the way I see it is we have really 2 options to fix this issue:
TransactionalPersistenceinterface to be stateless (either now or in the future). ORrealmContextto pass into the supplier that breaks the dependency on therealmContextthat originally came from the function signature. While this is a less invasive change, I do not have an easy way to test this behavior.I'm leaning towards option 2 simply because it is less invasive - but is everyone else okay without there being hard testing for this few-line change? It would look something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question and it's not as straightforward as it might seem. The
RealmContextinterface only defines agetNamemethod, but there are concrete implementations that may contain extra information about the realm (we have our own custom impl). Simply materializing theRealmContextin this way could break functionality if the underlying Session/MetaStoreManager depend on the concrete implementation.I think the proper long-term fix is to make the
BasePersistenceitself a CDI-managed bean so that theRealmContextcan be injected by the context rather than us materializing it manually. It also means we have to make the task execution framework CDI-managed, which is a bigger task that we've been putting off for a while