ABR 18: More ETEs #5911
ABR 18: More ETEs #5911
Conversation
Generate changelog in
|
| import com.palantir.atlasdb.transaction.api.TransactionManager; | ||
| import java.util.function.Function; | ||
|
|
||
| final class TransactionManagerKvsRunner implements KvsRunner { |
There was a problem hiding this comment.
TransactionManagerScopedKvsRunner?
There was a problem hiding this comment.
I meant more agnostic and descriptive of behaviour/intent: SharedKvsRunner or even NonClosingKvsRunner?
timelock-impl/src/main/java/com/palantir/atlasdb/timelock/TimelockNamespaces.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public <T> T run(Namespace namespace, Function<KeyValueService, T> function) { | ||
| try (KeyValueService kvs = keyValueServiceFactory.apply(namespace)) { |
There was a problem hiding this comment.
Seems a bit excessive to create and close on every call -- unless we really only do it once. We should Ideally just make the service closeable and close the KVS on close if necessary
There was a problem hiding this comment.
For a given namespace, the KVS is indeed created and fetched exactly once per backup or restore.
| @@ -79,7 +80,7 @@ public TimeLockServices get(String namespace) { | |||
| } | |||
|
|
|||
| public TimeLockServices getIgnoringDisabled(String namespace) { | |||
| return services.computeIfAbsent(namespace, ns -> createNewClient(ns, true)); | |||
| return Optional.ofNullable(services.get(namespace)).orElseGet(() -> createNewClient(namespace, true)); | |||
There was a problem hiding this comment.
This solves the correctness issue, but it potentially creates a client on every call and closes any of them. One way to fix it is just to have a separate ConcurrentMap for the ignoring services and not reuse the existing normal services.
Alternatively, and maybe better, we can keep the previous behaviour where an explicit disable still kills the existing service (because it does all the locks etc cleanup for us) and let the ignoring version create a new one but make sure that a get does a precondition check to verify it's not disabled after calling services.computeIfAbsent
There was a problem hiding this comment.
Ah interesting - yeah, that would work. So we'd create a new service the first time we call getIgnoringDisabled, then future get calls with throw until the service is re-enabled.
| @@ -76,11 +75,16 @@ public TimelockNamespaces( | |||
| } | |||
|
|
|||
| public TimeLockServices get(String namespace) { | |||
| Preconditions.checkArgument( | |||
There was a problem hiding this comment.
It must go after we computeIfAbsent, to avoid race conditions where this passes, we disable and call getIgnoring and then this calls computeIfAbsent
|
Released 0.548.0 |
This reverts commit 06dbd5b.
Goals (and why):
Add further ETE tests, fixing a few problems along the way.
Notably:
TimelockManagementServicewhile the namespace is disabled. To achieve this, I addedTimelockNamespaces.getIgnoringDisabled, to bypass the check in client creation. This means that other code paths using TMS would also work while the namespace is disabled; but since this interface only containspingandfastForwardTimestamp, this should be safe (for reasonable timestamp values).CoordinationServiceRecorder/CassandraRepairHelper, we use aKeyValueServiceinstance. In the production codepath, these will be newly created, and thus we must use try-with-resources to avoid leaking resources. However, the ETE test codepath uses the KVS from theTransactionManager, which must not be closed. To fix this issue, I pulled out a littleKvsRunnerclass.==COMMIT_MSG==
Fixed an issue where the AtlasRestoreClient would be unable to perform necessary operations on timelock (fast forwarding the timestamp) during the restore process.
==COMMIT_MSG==
Implementation Description (bullets):
Testing (What was existing testing like? What have you done to improve it?):
Concerns (what feedback would you like?): See the notable things above
Where should we start reviewing?: For maximum 🌶️ , KvsRunner + friends, followed by TimelockNamespaces.
Priority (whenever / two weeks / yesterday): this week