mark RangeRequest.all() as @RestrictedApi #7375

bavardage · 2024-10-18T17:33:41Z

this means callers have to justify their use of full range requests

After this PR:

==COMMIT_MSG==
Mark RangeRequest.all as @RestrictedApi
==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
@raiju

this means callers have to justify their use of full range requests

changelog-app · 2024-10-18T17:33:45Z

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

Mark RangeRequest.all as @RestrictedApi

Check the box to generate changelog(s)

Generate changelog entry

bavardage

don't have context on some existing uses of RangeRequest.all()

bavardage · 2024-10-18T17:58:21Z

...ndra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceImpl.java

@@ -1691,6 +1692,7 @@ private static String schemaChangeDescriptionForPutMetadataForTables(Collection<
    }

    @Override
+    @AllowedRangeRequest(justification = "RangeRequest.all only invoked for equality check")


but is truncateTables bad/expensive?

The method checks if the request is equal to rangerequest.all() and in that case, as an optimisation, truncated the table instead of writing deletes on the full range. Truncate does not create tombstones so in that perspective it's better, but not sure about the performance. @jeremyk-91 do you know if this is expensive?

let's make it consistent with the other place where the justification is same: "a full range request is constructed only for an equality check"

With regard to truncate: I'd check with @inespot, but my understanding is that yes, truncate is much cheaper than just deleting. It does require all Cassandra nodes to be up and the coordinator needs to get an ack from each node that the data was deleted, but this should be comparatively fast. Also, in practice, deletes generally require consistency all and will similarly be affected by single node degradation (strictly speaking, there is an edge case where deletion is probably faster, outlined below in (*), but I don't think we care in practice).

(*) The "normal" method involves reading all cells (which only requires consistency quorum) and then deleting these cells. That does require consistency all; but technically if there's a situation where no 2 nodes owning the same data are down (or are slow), and for rows that actually exist in the table, no single node owning the same data is down (or is slow), legacy works (or is fast) while truncate does not (or is slow). This is more of an academic edge case, though.

bavardage · 2024-10-18T17:58:50Z

...-impl-shared/src/main/java/com/palantir/atlasdb/restore/V1TransactionsTableRangeDeleter.java

@@ -47,6 +48,7 @@ public V1TransactionsTableRangeDeleter(
    }

    @Override
+    @AllowedRangeRequest(justification = "???")


any context on what's calling this/why doing this deletion is ok?

@jeremyk-91 is this on the restore path?

Yes, only used on the restore path and for the transaction table, which in normal operation does not have any tombstones written. (Yes, there is a risk if you restore a stack twice in succession with no intervening compaction; should we make an internal note of this on SOPs or similar?)

bavardage · 2024-10-18T17:58:58Z

...db-impl-shared/src/main/java/com/palantir/atlasdb/sweep/queue/clear/DefaultTableClearer.java

@@ -89,6 +90,7 @@ private Set<TableReference> getConservativeTables(Set<TableReference> tables) {
    }

    @Override
+    @AllowedRangeRequest(justification = "deleting a range is cheaper than reading one")


actually true?

truncating the table is a cheaper option to deleting the full range

bavardage · 2024-10-18T17:59:16Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/table/common/TableTasks.java

@@ -163,6 +164,7 @@ private static PartialCopyStats copyInternal(
        return stats;
    }

+    @AllowedRangeRequest(justification = "???")


size estimator is ok because..? we only ever read one batch?

ohh this is spicy, this in theory can be very expensive if for reading that one batch we need to contact all cassandra nodes.

Today at least: only used by the large internal product, which is not generally deployed with Cassandra.

(This probably should itself be restricted, though probably something for another day...)

sverma30

We should do an announcement before pushing out this change to lower the possibility of teams falling behind on upgrading AtlasDB and to make sure they understand why we're doing this.

sverma30 · 2024-10-21T20:04:49Z

...ndra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceImpl.java

@@ -1691,6 +1692,7 @@ private static String schemaChangeDescriptionForPutMetadataForTables(Collection<
    }

    @Override
+    @AllowedRangeRequest(justification = "RangeRequest.all only invoked for equality check")


The method checks if the request is equal to rangerequest.all() and in that case, as an optimisation, truncated the table instead of writing deletes on the full range. Truncate does not create tombstones so in that perspective it's better, but not sure about the performance. @jeremyk-91 do you know if this is expensive?

sverma30 · 2024-10-21T20:06:26Z

...ndra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceImpl.java

@@ -1691,6 +1692,7 @@ private static String schemaChangeDescriptionForPutMetadataForTables(Collection<
    }

    @Override
+    @AllowedRangeRequest(justification = "RangeRequest.all only invoked for equality check")


let's make it consistent with the other place where the justification is same: "a full range request is constructed only for an equality check"

sverma30 · 2024-10-21T20:07:55Z

...-impl-shared/src/main/java/com/palantir/atlasdb/restore/V1TransactionsTableRangeDeleter.java

@@ -47,6 +48,7 @@ public V1TransactionsTableRangeDeleter(
    }

    @Override
+    @AllowedRangeRequest(justification = "???")


@jeremyk-91 is this on the restore path?

sverma30 · 2024-10-21T20:09:00Z

...db-impl-shared/src/main/java/com/palantir/atlasdb/sweep/queue/clear/DefaultTableClearer.java

@@ -89,6 +90,7 @@ private Set<TableReference> getConservativeTables(Set<TableReference> tables) {
    }

    @Override
+    @AllowedRangeRequest(justification = "deleting a range is cheaper than reading one")


truncating the table is a cheaper option to deleting the full range

sverma30 · 2024-10-21T20:51:12Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/table/common/TableTasks.java

@@ -163,6 +164,7 @@ private static PartialCopyStats copyInternal(
        return stats;
    }

+    @AllowedRangeRequest(justification = "???")


ohh this is spicy, this in theory can be very expensive if for reading that one batch we need to contact all cassandra nodes.

sverma30 · 2024-10-21T21:17:23Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/AllowedRangeRequest.java

+/**
+ * Mark that this range request has been thought about and explicitly decided to be ok.
+ */
+public @interface AllowedRangeRequest {


I was wondering if having an enum field for high level categorisation would be helpful for future analysis for why services are relying on rangerequests. Like background task, search use case, backfill, etc?

interesting! could be useful, yes.

jeremyk-91

Thanks for writing this! Was interesting to refresh myself on the various contexts where this is used as well. @sverma30's enum suggestion would probably be nice, and nothing blocking on my end - I've mentioned some of the "answers" to the ??? statements which we should include.

jeremyk-91 · 2024-10-28T17:48:17Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/AllowedRangeRequest.java

+/**
+ * Mark that this range request has been thought about and explicitly decided to be ok.
+ */
+public @interface AllowedRangeRequest {


interesting! could be useful, yes.

jeremyk-91 · 2024-10-28T17:56:30Z

...ndra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceImpl.java

@@ -1691,6 +1692,7 @@ private static String schemaChangeDescriptionForPutMetadataForTables(Collection<
    }

    @Override
+    @AllowedRangeRequest(justification = "RangeRequest.all only invoked for equality check")


With regard to truncate: I'd check with @inespot, but my understanding is that yes, truncate is much cheaper than just deleting. It does require all Cassandra nodes to be up and the coordinator needs to get an ack from each node that the data was deleted, but this should be comparatively fast. Also, in practice, deletes generally require consistency all and will similarly be affected by single node degradation (strictly speaking, there is an edge case where deletion is probably faster, outlined below in (*), but I don't think we care in practice).

(*) The "normal" method involves reading all cells (which only requires consistency quorum) and then deleting these cells. That does require consistency all; but technically if there's a situation where no 2 nodes owning the same data are down (or are slow), and for rows that actually exist in the table, no single node owning the same data is down (or is slow), legacy works (or is fast) while truncate does not (or is slow). This is more of an academic edge case, though.

jeremyk-91 · 2024-10-28T21:48:56Z

atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/table/common/TableTasks.java

@@ -163,6 +164,7 @@ private static PartialCopyStats copyInternal(
        return stats;
    }

+    @AllowedRangeRequest(justification = "???")


Today at least: only used by the large internal product, which is not generally deployed with Cassandra.

(This probably should itself be restricted, though probably something for another day...)

jeremyk-91 · 2024-10-28T21:50:53Z

...-impl-shared/src/main/java/com/palantir/atlasdb/restore/V1TransactionsTableRangeDeleter.java

@@ -47,6 +48,7 @@ public V1TransactionsTableRangeDeleter(
    }

    @Override
+    @AllowedRangeRequest(justification = "???")


Yes, only used on the restore path and for the transaction table, which in normal operation does not have any tombstones written. (Yes, there is a risk if you restore a stack twice in succession with no intervening compaction; should we make an internal note of this on SOPs or similar?)

jeremyk-91 · 2024-10-28T21:51:15Z

atlasdb-api/src/main/java/com/palantir/atlasdb/keyvalue/api/AllowedRangeRequest.java

+     * know that the overall number of rows read will be small, etc.
+     */
+    String justification();
+}


nit: newline

mark RangeRequest.all() as @RestrictedApi

a178ae3

this means callers have to justify their use of full range requests

bavardage added 2 commits October 18, 2024 13:40

some justifications

3f83e04

some justifications

7a319e7

bavardage commented Oct 18, 2024

View reviewed changes

sverma30 reviewed Oct 21, 2024

View reviewed changes

jeremyk-91 reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mark RangeRequest.all() as @RestrictedApi #7375

mark RangeRequest.all() as @RestrictedApi #7375

bavardage commented Oct 18, 2024

changelog-app bot commented Oct 18, 2024

bavardage left a comment

bavardage Oct 18, 2024

sverma30 Oct 21, 2024

sverma30 Oct 21, 2024

jeremyk-91 Oct 28, 2024 •

edited

Loading

bavardage Oct 18, 2024

sverma30 Oct 21, 2024

jeremyk-91 Oct 28, 2024

bavardage Oct 18, 2024

sverma30 Oct 21, 2024

bavardage Oct 18, 2024

sverma30 Oct 21, 2024

jeremyk-91 Oct 28, 2024

sverma30 left a comment

sverma30 Oct 21, 2024

sverma30 Oct 21, 2024

sverma30 Oct 21, 2024

sverma30 Oct 21, 2024

sverma30 Oct 21, 2024

sverma30 Oct 21, 2024 •

edited

Loading

jeremyk-91 Oct 28, 2024

jeremyk-91 left a comment

jeremyk-91 Oct 28, 2024

jeremyk-91 Oct 28, 2024 •

edited

Loading

jeremyk-91 Oct 28, 2024

jeremyk-91 Oct 28, 2024

jeremyk-91 Oct 28, 2024

mark RangeRequest.all() as @RestrictedApi #7375

Are you sure you want to change the base?

mark RangeRequest.all() as @RestrictedApi #7375

Conversation

bavardage commented Oct 18, 2024

Compatibility

Testing and Correctness

Execution

Scale

Development Process

changelog-app bot commented Oct 18, 2024

Generate changelog in changelog/@unreleased

bavardage 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

jeremyk-91 Oct 28, 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

sverma30 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

sverma30 Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyk-91 Oct 28, 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

Generate changelog in `changelog/@unreleased`

jeremyk-91 Oct 28, 2024 •

edited

Loading

sverma30 Oct 21, 2024 •

edited

Loading

jeremyk-91 Oct 28, 2024 •

edited

Loading