Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

[ASTS] Add getSweepableBucket method #7440

Open
wants to merge 1 commit into
base: mdaudali/11-12-_asts_fix_update_gettimestamprangerecord_to_return_empty_when_record_not_present
Choose a base branch
from

Conversation

mdaudali
Copy link
Contributor

General

Before this PR:
See #7439

After this PR:
Adds a method to get the sweepable bucket (so that the background task can check if it exists, and if it's open, what the start timestamp of the bucket is)

==COMMIT_MSG==
==COMMIT_MSG==

Priority:
P2
Concerns / possible downsides (what feedback would you like?):
N/A
Is documentation needed?:
No

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?:
No
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?:
Updated tests
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?:
N/A

Execution

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

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

@mdaudali mdaudali force-pushed the mdaudali/11-12-_asts_add_getsweepablebucket_method branch from 708d853 to b9e71ca Compare November 12, 2024 16:22
Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -272,8 +286,7 @@ public void putTimestampRangeForBucketSucceedsIfOldTimestampRangeMatchesCurrent(
TimestampRange newTimestampRange = TimestampRange.of(1, 2);

store.putTimestampRangeForBucket(bucket, Optional.empty(), newTimestampRange);
Set<SweepableBucket> sweepableBuckets = store.getSweepableBuckets(Set.of(bucket));
assertThat(sweepableBuckets).containsExactly(SweepableBucket.of(bucket, newTimestampRange));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thank you for updating this!

@mdaudali mdaudali force-pushed the mdaudali/11-12-_asts_fix_update_gettimestamprangerecord_to_return_empty_when_record_not_present branch from 26515f9 to f7f270b Compare November 13, 2024 17:51
@mdaudali mdaudali force-pushed the mdaudali/11-12-_asts_add_getsweepablebucket_method branch from b9e71ca to 7064061 Compare November 13, 2024 17:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants