mark RangeRequest.all() as @RestrictedApi #7375
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /* | ||
| * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. | ||
| * | ||
| * Licensed 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 com.palantir.atlasdb.keyvalue.api; | ||
|
|
||
| /** | ||
| * Mark that this range request has been thought about and explicitly decided to be ok. | ||
| */ | ||
| public @interface AllowedRangeRequest { | ||
| /** | ||
| * Explain why this is ok - i.e. what rate limiting is in place on calling codepaths/how we | ||
| * know that the overall number of rows read will be small, etc. | ||
| */ | ||
| String justification(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| import com.palantir.atlasdb.cassandra.CassandraMutationTimestampProviders; | ||
| import com.palantir.atlasdb.cassandra.CassandraServersConfigs.CassandraServersConfig; | ||
| import com.palantir.atlasdb.encoding.PtBytes; | ||
| import com.palantir.atlasdb.keyvalue.api.AllowedRangeRequest; | ||
| import com.palantir.atlasdb.keyvalue.api.AsyncKeyValueService; | ||
| import com.palantir.atlasdb.keyvalue.api.BatchColumnRangeSelection; | ||
| import com.palantir.atlasdb.keyvalue.api.CandidateCellForSweeping; | ||
|
|
@@ -1691,6 +1692,7 @@ private static String schemaChangeDescriptionForPutMetadataForTables(Collection< | |
| } | ||
|
|
||
| @Override | ||
| @AllowedRangeRequest(justification = "RangeRequest.all only invoked for equality check") | ||
|
There was a problem hiding this comment. but is truncateTables bad/expensive? There was a problem hiding this comment. 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? There was a problem hiding this comment. 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" There was a problem hiding this comment. 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. |
||
| public void deleteRange(final TableReference tableRef, final RangeRequest range) { | ||
| if (range.equals(RangeRequest.all())) { | ||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
|
|
||
| import com.google.common.collect.HashMultimap; | ||
| import com.google.common.collect.Multimap; | ||
| import com.palantir.atlasdb.keyvalue.api.AllowedRangeRequest; | ||
| import com.palantir.atlasdb.keyvalue.api.Cell; | ||
| import com.palantir.atlasdb.keyvalue.api.KeyValueService; | ||
| import com.palantir.atlasdb.keyvalue.api.RangeRequest; | ||
|
|
@@ -47,6 +48,7 @@ public V1TransactionsTableRangeDeleter( | |
| } | ||
|
|
||
| @Override | ||
| @AllowedRangeRequest(justification = "???") | ||
|
There was a problem hiding this comment. any context on what's calling this/why doing this deletion is ok? There was a problem hiding this comment. @jeremyk-91 is this on the restore path? There was a problem hiding this comment. 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?) |
||
| public void deleteRange(TimestampRange commitTimestampRange) { | ||
| byte[] startBytes = TransactionConstants.getValueForTimestamp( | ||
| startTimestamp.orElse(TransactionConstants.LOWEST_POSSIBLE_START_TS)); | ||
|
|
||
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 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?
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.
interesting! could be useful, yes.