[Named min timestamp leases] Acquire single lease per transaction-shaped batch #7385

ergo14 · 2024-10-21T07:50:05Z

General

Before this PR:

After this PR:
API changes for timestamp leases.
==COMMIT_MSG==
==COMMIT_MSG==

Priority:
P1
Concerns / possible downsides (what feedback would you like?):
See PR comments
Is documentation needed?:

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?:
Yes. API unused.
Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?:
No.
The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.):
Yes
Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?:
No
Does this PR need a schema migration?
No

Testing and Correctness

What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?:
N/A
What was existing testing like? What have you done to improve it?:
Changed
If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.:
N/A
If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?:
See PR comments

Execution

How would I tell this PR works in production? (Metrics, logs, etc.):
No op
Has the safety of all log arguments been decided correctly?:
Yes
Will this change significantly affect our spending on metrics or logs?:
No
How would I tell that this PR does not work in production? (monitors, etc.):
No op
If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?:
Rollback
If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC):

Scale

Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.:
No
Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?:
No
Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?:
No

Development Process

Where should we start reviewing?:

If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?:

Please tag any other people who should be aware of this PR:
@jeremyk-91
@raiju

timelock-api/src/main/conjure/timelock-api.yml

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/AsyncTimelockServiceImpl.java

ergo14 · 2024-10-21T07:58:08Z

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/AsyncLockService.java

-            TimestampLeaseName timestampName, UUID requestId, long timestamp) {
-        AsyncLock lock = lockManager.getNamedTimestampLock(timestampName, timestamp);
-        return lockAcquirer.acquireLocks(requestId, OrderedLocks.fromSingleLock(lock), TimeLimit.zero());
+            UUID requestId, Set<TimestampLeaseName> timestampNames, long timestamp) {


This is a set because the ordering doesn't matter.

In HeldLocks, the list is just used to be iterated on in lock/unlock operations. And, the lock descriptors are returned as a set.

Ah, looks like we thought about the same thing. I agree it doesn't matter, but it definitely reads weird. So maybe to allay these concerns, add an explicit comment OR handle the ordering.

jkozlowski · 2024-10-21T09:16:45Z

timelock-api/src/main/conjure/timelock-api.yml

      TimestampLeaseRequests:
-         alias: map<TimestampLeaseName, TimestampLeaseRequest>
+          fields:
+            requestsId: RequestId


requestId! 😅

Fixed in one of the PRs that merged - might be worth to go synchronously on the final yml api

jkozlowski · 2024-10-21T09:21:38Z

timelock-api/src/main/conjure/timelock-api.yml

+          fields:
+            requestsId: RequestId
+            numFreshTimestamps: map<TimestampLeaseName, integer>
+      TimestampLeasesRequest:


Dunno if we should do something like:

MultiClientAcquireTimestampLeasesRequest

NamespaceAcquireTimestampLeasesRequest

AcquireTimestampLeasesRequest

I think this sets the right hierarchy or Multi-Client -> Namespace -> Single request

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/AsyncLockService.java

jkozlowski · 2024-10-21T09:29:46Z

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/AsyncLockService.java

@@ -177,9 +179,11 @@ private AsyncResult<HeldLocks> acquireImmutableTimestampLock(UUID requestId, lon
    }

    private AsyncResult<HeldLocks> acquireNamedTimestampLockInternal(


acquireNamedTimestampLocksInternal

In general, let's make sure you do a naming-alignment pass once we're done with all of the changes. I think we've iterated quite hard, so the whole chain is out of whack.

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/AsyncTimelockServiceImpl.java

jkozlowski · 2024-10-21T09:32:56Z

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/AsyncTimelockServiceImpl.java

                .get();
-        long minLeased = lockService.getMinLeasedTimestamp(timestampName).orElse(timestamp);



Let's add a comment here that it's crucial that the timestamps here are AFTER the fresh timestamp we're locking on.

jkozlowski

Just some naming shenanigans, and cleanup

ergo14 · 2024-10-21T15:22:12Z

I put all the FLUPs in #7386 which I will merge soon.

jeremyk-91

Directionally in sync! Bunch of implementation concerns, and the Lease(s?)Response(s?) naming is tricky but we can decide that a bit more synchronously probably (and no need to block on that)

jeremyk-91 · 2024-10-29T16:43:04Z

timelock-api/src/main/conjure/timelock-api.yml

      TimestampLeaseRequests:
-         alias: map<TimestampLeaseName, TimestampLeaseRequest>
+          fields:
+            requestsId: RequestId


requestId! 😅

jeremyk-91 · 2024-10-29T16:51:11Z

timelock-impl/src/test/java/com/palantir/atlasdb/timelock/AsyncTimelockServiceImplTest.java

+                .collect(Collectors.toList());
+    }
+
+    private static void assertThatTimestampsIsStrictlyWithinInvocationInterval(


pedantry: timestamps are strictly within.

Though 👍 on use of allSatisfy - it gives better error messages than going forEach on the timestamps :)

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/AsyncLockService.java

jeremyk-91 · 2024-10-29T16:58:06Z

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/AsyncLockService.java

+        List<AsyncLock> locks = timestampNames.stream()
+                .map(name -> lockManager.getNamedTimestampLock(name, timestamp))
+                .collect(Collectors.toList());
+        return lockAcquirer.acquireLocks(requestId, OrderedLocks.fromOrderedList(locks), TimeLimit.zero());


Agree this works as written because the lock descriptor has a fresh timestamp appended, and so there should never be contention. A comment here is IMO required though: if the locks provided here actually had contention this is not OK because of the possibility of deadlock.

(Curious if there's more context elsewhere!)

jeremyk-91 · 2024-10-29T16:58:39Z

.../src/main/java/com/palantir/atlasdb/timelock/batch/RemotingTimestampLeaseServiceAdapter.java

-            AsyncTimelockService service, TimestampLeaseName timestampName, TimestampLeaseRequest request) {
-        return service.acquireTimestampLease(timestampName, request.getRequestId(), request.getNumFreshTimestamps());
+    private static ListenableFuture<TimestampLeaseResponses> acquireTimestampLease(
+            AsyncTimelockService service, RequestId requestsId, Map<TimestampLeaseName, Integer> numFreshTimestamps) {


pedantry: requestId

jeremyk-91 · 2024-10-29T16:58:47Z

...ava/com/palantir/atlasdb/timelock/batch/RemotingMultiClientTimestampLeaseServiceAdapter.java

@@ -46,7 +46,7 @@ private RemotingMultiClientTimestampLeaseServiceAdapter(RemotingTimestampLeaseSe

    ListenableFuture<MultiClientTimestampLeaseResponse> acquireTimestampLeases(
            MultiClientTimestampLeaseRequest requests, @Nullable RequestContext context) {
-        Map<Namespace, ListenableFuture<TimestampLeaseResponses>> futures = KeyedStream.stream(requests.get())
+        Map<Namespace, ListenableFuture<TimestampLeasesResponse>> futures = KeyedStream.stream(requests.get())


jeremyk-91 · 2024-10-29T17:03:22Z

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/AsyncTimelockServiceImpl.java

@@ -337,6 +338,17 @@ public void logState() {
        lockService.getLockWatchingService().logState();
    }

+    private TimestampLeaseResponse getMinLeasedAndFreshTimestamps(
+            TimestampLeaseName timestampName, int numFreshTimestamps, long lockedTimestamp) {


Is this the right factoring? There's value in calling getFreshTimestamps once for the N lease names and then portioning the timestamps out across the things that want it, after having gotten the min leased / locked timestamps for each of the namespaces in that we save a bunch of volatile reads and writes - normally I wouldn't care as much, but this is timelock...

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/AsyncTimelockServiceImpl.java

jeremyk-91 · 2024-10-29T17:05:36Z

timelock-api/src/main/conjure/timelock-api.yml

      MultiClientTimestampLeaseResponse:
-         alias: map<Namespace, TimestampLeaseResponses>
+         alias: map<Namespace, TimestampLeasesResponse>


oof, yeah this is a bit tricky to follow. We have LeasesResponse having a bunch of LeaseResponses. Also, I do think LeasesResponse should probably be the internal and this one should be LeasesResponses. Though if you changed the naming later would be curious to see what you ended up with.

Acquire single timestamp lease per transaction-shaped batch

5cf2947

ergo14 commented Oct 21, 2024

View reviewed changes

ergo14 marked this pull request as ready for review October 21, 2024 07:58

ergo14 changed the title ~~Acquire single timestamp lease per transaction-shaped batch~~ [Named min timestamp Leases] Acquire single lease per transaction-shaped batch Oct 21, 2024

jkozlowski reviewed Oct 21, 2024

View reviewed changes

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/lock/AsyncLockService.java Show resolved Hide resolved

jkozlowski reviewed Oct 21, 2024

View reviewed changes

timelock-impl/src/main/java/com/palantir/atlasdb/timelock/AsyncTimelockServiceImpl.java Show resolved Hide resolved

jkozlowski reviewed Oct 21, 2024

View reviewed changes

jkozlowski added the do not merge label Oct 21, 2024

jkozlowski approved these changes Oct 21, 2024

View reviewed changes

ergo14 added no changelog merge when ready and removed do not merge labels Oct 21, 2024

bulldozer-bot bot merged commit aa8665f into develop Oct 21, 2024
21 checks passed

ergo14 changed the title ~~[Named min timestamp Leases] Acquire single lease per transaction-shaped batch~~ [Named min timestamp leases] Acquire single lease per transaction-shaped batch Oct 22, 2024

jeremyk-91 reviewed Oct 29, 2024

View reviewed changes

ergo14 added a commit that referenced this pull request Oct 31, 2024

[Named min timestamp leases] Post merge review #7385

4826b68

changelog-app bot mentioned this pull request Oct 31, 2024

[Named min timestamp leases] Post merge review #7385 #7417

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Named min timestamp leases] Acquire single lease per transaction-shaped batch #7385

[Named min timestamp leases] Acquire single lease per transaction-shaped batch #7385

ergo14 commented Oct 21, 2024 •

edited

Loading

ergo14 Oct 21, 2024

jkozlowski Oct 21, 2024

jkozlowski Oct 21, 2024

jeremyk-91 Oct 29, 2024

ergo14 Oct 31, 2024

jkozlowski Oct 21, 2024

jkozlowski Oct 21, 2024 •

edited

Loading

jkozlowski Oct 21, 2024

jkozlowski left a comment

ergo14 commented Oct 21, 2024

jeremyk-91 left a comment

jeremyk-91 Oct 29, 2024

jeremyk-91 Oct 29, 2024

jeremyk-91 Oct 29, 2024 •

edited

Loading

jeremyk-91 Oct 29, 2024

jeremyk-91 Oct 29, 2024

jeremyk-91 Oct 29, 2024

jeremyk-91 Oct 29, 2024

		@@ -177,9 +179,11 @@ private AsyncResult<HeldLocks> acquireImmutableTimestampLock(UUID requestId, lon
		}

		private AsyncResult<HeldLocks> acquireNamedTimestampLockInternal(

		.get();
		long minLeased = lockService.getMinLeasedTimestamp(timestampName).orElse(timestamp);

[Named min timestamp leases] Acquire single lease per transaction-shaped batch #7385

[Named min timestamp leases] Acquire single lease per transaction-shaped batch #7385

Conversation

ergo14 commented Oct 21, 2024 • edited Loading

General

Compatibility

Testing and Correctness

Execution

Scale

Development Process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkozlowski Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkozlowski left a comment

Choose a reason for hiding this comment

ergo14 commented Oct 21, 2024

jeremyk-91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ergo14 commented Oct 21, 2024 •

edited

Loading

jkozlowski Oct 21, 2024 •

edited

Loading

jeremyk-91 Oct 29, 2024 •

edited

Loading