Add Lease Timestamps #7305

fsamuel-bs · 2024-10-01T13:38:53Z

General

Before this PR:
Workflows that attempted to implement a queue-like behavior with atlas timestamps did not perform well. The main issue was that they needed to scan from the immutable timestamp to account for written timestamps for ongoing transactions.

After this PR:
There's a new feature, called leased timestamps. With it, we know, for a given workflow, the minimum timestamp of open transactions. With this information, the range of timestamps we need to scan is significantly reduced.

==COMMIT_MSG==
==COMMIT_MSG==

Priority:

Concerns / possible downsides (what feedback would you like?):

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?:

Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?:

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.):

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)?:

Does this PR need a schema migration?

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?:

What was existing testing like? What have you done to improve it?:

If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.:

If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?:

Execution

How would I tell this PR works in production? (Metrics, logs, etc.):

Has the safety of all log arguments been decided correctly?:

Will this change significantly affect our spending on metrics or logs?:

How would I tell that this PR does not work in production? (monitors, etc.):

If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?:

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.:

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)?:

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?:

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
@sverma30
@raiju

changelog-app · 2024-10-01T13:38:57Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Add leased timestamps to TimestampLeaseAwareTransaction and TimestampLeaseAwareTransactionManager APIs.

Check the box to generate changelog(s)

Generate changelog entry

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java

jkozlowski · 2024-10-01T14:19:38Z

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java

+     * @throws RuntimeException If requesting more timestamps in {@code preCommitAction} than specified in
+     * timestampCount.
+     */
+    void preCommit(String timestampLockDescriptor, int timestampCount, Consumer<TimestampSupplier> preCommitAction);


PreCommitCondition is already a thing in AtlasDB, does the naming not clash here?

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java

jkozlowski · 2024-10-01T14:24:15Z

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java

+     * Clients can use {@link #getLockedTimestamp(String)} to fetch the earliest locked
+     * timestamp for a given {@param timestampLockDescriptor} of an open transaction.
+     * Note these semantics are the quite similar to {@link TransactionManager#getImmutableTimestamp()}, but instead
+     * of tracking open start transaction timestamps, we track open {@code preCommitAction} timestamps.


As discussed: the consequence of this API is that you EXPECT the user to just call into their stores (and by extension the Transaction in AtlasDB) so this NEEDs to happen before the code in SnapshotTransaction#commitWrites or something.

Is there a usecase that you had for this style of work outside of this workstream? Because I struggle to understand why this is better than the more generic idea of "delayed-writes" (and not allowing people to interact with their Transaction object at the point where that function is ran).

Additional concern I'd have is: would people expect that, if they happen to use their Transaction object from multiple threads, that there's an ordering between all interactions with the transaction object happened AND THEN these runnables are ran?

So concretely, would we want to have another state in https://github.com/palantir/atlasdb/blob/develop/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java#L243 or something?

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java

fsamuel-bs · 2024-10-22T14:22:09Z

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java

+    static class PreCommitAction {
+        final Consumer<LongSupplier> action;
+        final int timestampCount;
+
+        PreCommitAction(Consumer<LongSupplier> action, int timestampCount) {
+            this.action = action;
+            this.timestampCount = timestampCount;
+        }
+    }
+
+    static class PerLeaseActions {
+        final List<PreCommitAction> runnables = new ArrayList<>();
+        int timestampCount = 0;
+    }


These ideally should be records, but Atlas is not on java 17 yet.

fsamuel-bs · 2024-10-22T14:22:37Z

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java

+        perLeaseActions.timestampCount += numLeasedTimestamps;
+        perLeaseActions.runnables.add(new PreCommitAction(action, numLeasedTimestamps));


Feels a bit wrong to be accessing nested elements like this. :/

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java

jkozlowski · 2024-10-23T10:51:08Z

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java

+     * for open transactions.
+     */
+    @Beta
+    long getLeasedTimestamp(TimestampLeaseName leaseName);


Adding this here will break upgrades for people, I suspect? or be a runtime failure?

Should we consider the same underhanded trick you're doing for TimestampLeaseAwareTransaction?

:(. Ya, there are some internal dependencies. Let me fix this then.

jkozlowski · 2024-10-23T10:53:03Z

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java

+     * If no transactions with a {@code timestampLeaseName} lock are open, this method returns a new fresh timestamp
+     * (i.e. equivalent to {@link TimelockService#getFreshTimestamp()}).
+     * <p>
+     * Consumers should to fetch the leased timestamp outside of transactions that potentially use it - if fetching the


I would say this is maybe a useful warning, but it assumes a lot about how this is used, therefore possibly making it not worth putting in here?

Let's be bias towards over-communicating - this feature is too complex already.

jkozlowski · 2024-10-23T10:54:10Z

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java

+     * (i.e. equivalent to {@link TimelockService#getFreshTimestamp()}).
+     * <p>
+     * Consumers should to fetch the leased timestamp outside of transactions that potentially use it - if fetching the
+     * leased timestamp inside a transaction, it's possible for the transaction's start timestamp < leased timestamp,


Should we add a warning that this is a very advanced feature and nobody should use this without TS involvement. Also, a warning that this timestamp MAY sometimes go back (if observed globally)

I'll just do the RestrictedApi thing then.

jkozlowski · 2024-10-23T10:58:36Z

atlasdb-impl-shared/build.gradle

@@ -17,6 +17,7 @@ license {
 }

 dependencies {
+  api project(":timelock-api")


Yeah. Understood where this comes from, and I think in general this is fine, but I also don't understand exactly how these subprojects are decomped. Specifically, at least in the past, internal products would run without timelock.

Should we duplicate TimestampLeaseName in atlasdb-api instead of doing this?

No, it's too confusing internally. It's a pet peeve of mine to not have duplicate things flying around.

I understand that, but libraries are hard

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java

autorelease3 · 2024-10-28T13:53:10Z

Released 0.1180.0

jeremyk-91

The general design looks right, though I think some things were rushed in the name of speed, and we should probably come back to them.

Some of the implementation details of TPCA also seem a bit unexpected, so would be worth checking if they need to be exactly as written as well.

Also, please write a description! I know there is a lot of context on this feature currently, but given some of the other constraints we have, that might change.

...mmons-api/src/test/java/com/palantir/atlasdb/common/api/timelock/TimestampLeaseNameTest.java

jeremyk-91 · 2024-10-29T14:24:00Z

lock-impl/build.gradle

@@ -1,6 +1,7 @@
 apply from: "../gradle/shared.gradle"

 dependencies {
+  api project(":atlasdb-commons-api")


There's probably a reason we need this, but it's not immediately obvious to me why this is necessary over and above, e.g. timelock-api.

This really should be in the description.

We moved TimestampLeaseName to :atlasdb-commons-api, which is used in lock-impl. Our gradle plugin orchestration requires all dependencies to be explicitly declared.

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java

...rc/main/java/com/palantir/atlasdb/transaction/api/TimestampLeaseAwareTransactionManager.java

jeremyk-91 · 2024-10-29T14:44:00Z

...rc/main/java/com/palantir/atlasdb/transaction/api/TimestampLeaseAwareTransactionManager.java

+     * meaning the transaction cannot read all data up to leased timestamp.
+     *
+     * @param leaseName the name of the lease the timestamps are bound to
+     * @return the timestamp that is before any timestamp returned by the consumer of {@link TimestampLeaseAwareTransaction#preCommit(TimestampLeaseName, int, Consumer)}


...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java

jeremyk-91 · 2024-10-29T14:45:45Z

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java

+                        throw new SafeRuntimeException(
+                                "Cannot fetch leased timestamps since pre-commit action requested 0 leased timestamps",
+                                SafeArg.of("leaseName", timestampLeaseName));
+                    };


question: we have the 0 case for some internal usage? It seems that to add an action via preCommit, you're not allowed for this to be zero

We have theoretical ones, but I'll expose it as a new public method in Transaction with simpler signature - only preCommit(Runnable).

jkozlowski reviewed Oct 1, 2024

View reviewed changes

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 1, 2024

View reviewed changes

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 1, 2024

View reviewed changes

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 1, 2024

View reviewed changes

jkozlowski reviewed Oct 4, 2024

View reviewed changes

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 7, 2024

View reviewed changes

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 7, 2024

View reviewed changes

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 7, 2024

View reviewed changes

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 7, 2024

View reviewed changes

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java Outdated Show resolved Hide resolved

fsamuel-bs mentioned this pull request Oct 7, 2024

Call SnapshotTransaction#close() on OpenTransaction codepath #7322

Merged

fsamuel-bs force-pushed the ssouza/lock-aware-txn branch from b0f80eb to a5f77ff Compare October 22, 2024 10:50

jkozlowski reviewed Oct 22, 2024

View reviewed changes

...db-api/src/main/java/com/palantir/atlasdb/transaction/api/TimestampLockAwareTransaction.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 22, 2024

View reviewed changes

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/TransactionManager.java Outdated Show resolved Hide resolved

fsamuel-bs commented Oct 22, 2024

View reviewed changes

fsamuel-bs force-pushed the ssouza/lock-aware-txn branch 2 times, most recently from 7d87249 to 87c0469 Compare October 22, 2024 14:20

fsamuel-bs commented Oct 22, 2024

View reviewed changes

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java Outdated Show resolved Hide resolved

fsamuel-bs requested a review from jkozlowski October 23, 2024 10:41

fsamuel-bs marked this pull request as ready for review October 23, 2024 10:41

fsamuel-bs changed the title ~~LockAware Timestamps~~ LeaseAware Timestamps Oct 23, 2024

fsamuel-bs changed the title ~~LeaseAware Timestamps~~ Add Lease Timestamps Oct 23, 2024

jkozlowski reviewed Oct 23, 2024

View reviewed changes

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java Outdated Show resolved Hide resolved

jkozlowski reviewed Oct 23, 2024

View reviewed changes

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java Show resolved Hide resolved

jkozlowski reviewed Oct 23, 2024

View reviewed changes

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java Show resolved Hide resolved

jkozlowski reviewed Oct 23, 2024

View reviewed changes

...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java Show resolved Hide resolved

jkozlowski reviewed Oct 23, 2024

View reviewed changes

...-shared/src/main/java/com/palantir/atlasdb/transaction/impl/TransactionPreCommitActions.java Outdated Show resolved Hide resolved

fsamuel-bs mentioned this pull request Oct 24, 2024

Add new atlasdb-commons-api package #7399

Merged

fsamuel-bs force-pushed the ssouza/lock-aware-txn branch 3 times, most recently from 23543fa to 497c720 Compare October 25, 2024 12:42

leased timestamps

4099756

fsamuel-bs force-pushed the ssouza/lock-aware-txn branch from b29dd9d to 4099756 Compare October 25, 2024 15:59

fsamuel-bs added 3 commits October 25, 2024 17:05

also move test

cf6123f

fix...

303a817

Merge branch 'develop' into ssouza/lock-aware-txn

9771f6a

jkozlowski approved these changes Oct 25, 2024

View reviewed changes

jkozlowski added autorelease merge when ready labels Oct 25, 2024

fsamuel-bs added 5 commits October 25, 2024 17:52

fix more

4de9783

poke

f591db7

poke

11d2161

poke

996d935

poke

f7d4c9d

bulldozer-bot bot merged commit 9afedb1 into develop Oct 28, 2024
21 checks passed

bulldozer-bot bot deleted the ssouza/lock-aware-txn branch October 28, 2024 13:53

jeremyk-91 reviewed Oct 29, 2024

View reviewed changes

fsamuel-bs mentioned this pull request Oct 30, 2024

Lease timestamps follow ups #7412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Lease Timestamps #7305

Add Lease Timestamps #7305

fsamuel-bs commented Oct 1, 2024 •

edited

Loading

changelog-app bot commented Oct 1, 2024 •

edited by fsamuel-bs

Loading

jkozlowski Oct 1, 2024

jkozlowski Oct 1, 2024

jkozlowski Oct 1, 2024

fsamuel-bs Oct 22, 2024

fsamuel-bs Oct 22, 2024

jkozlowski Oct 23, 2024 •

edited

Loading

jkozlowski Oct 23, 2024

fsamuel-bs Oct 24, 2024

jkozlowski Oct 23, 2024

fsamuel-bs Oct 24, 2024

jkozlowski Oct 23, 2024

fsamuel-bs Oct 24, 2024

jkozlowski Oct 23, 2024

fsamuel-bs Oct 24, 2024

jkozlowski Oct 24, 2024

autorelease3 bot commented Oct 28, 2024

jeremyk-91 left a comment

jeremyk-91 Oct 29, 2024

fsamuel-bs Oct 29, 2024

jeremyk-91 Oct 29, 2024

jeremyk-91 Oct 29, 2024

fsamuel-bs Oct 29, 2024

		perLeaseActions.timestampCount += numLeasedTimestamps;
		perLeaseActions.runnables.add(new PreCommitAction(action, numLeasedTimestamps));

Add Lease Timestamps #7305

Add Lease Timestamps #7305

Conversation

fsamuel-bs commented Oct 1, 2024 • edited Loading

General

Compatibility

Testing and Correctness

Execution

Scale

Development Process

changelog-app bot commented Oct 1, 2024 • edited by fsamuel-bs Loading

Generate changelog in changelog/@unreleased

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 23, 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

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

autorelease3 bot commented Oct 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsamuel-bs commented Oct 1, 2024 •

edited

Loading

changelog-app bot commented Oct 1, 2024 •

edited by fsamuel-bs

Loading

Generate changelog in `changelog/@unreleased`

jkozlowski Oct 23, 2024 •

edited

Loading