This PR is part of a series: see the prototype https://github.com/palantir/atlasdb/pull/7000 or the internal RFC for what all of the pieces together are expected to look like. I have tried to separate this feature into reasonably sized components as otherwise it'd probably have a ~5k~ 10K delta or so!
Before this PR:
TransactionKeyValueService supports both get() and getAsync(). These have the same signature, except get() returns a Map<Cell, Value> while getAsync() returns a ListenableFuture<Map<Cell, Value>>.
After this PR:
==COMMIT_MSG==
TransactionKeyValueService only supports getAsync(). Users of get() should just call AtlasFuture.getUnchecked(...) or similar on the future returned from getAsync().
==COMMIT_MSG==
Priority: High P2 - nothing in particular but the workstream is a high priority.
Concerns / possible downsides (what feedback would you like?):
Is the overhead of managing futures too much? I think not (this is Atlas client side, so not necessarily an extremely hot code path).
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?: YES - but this interface is only used by internal migration tooling and Atlas itself
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?: That getAsync() and get() have the same semantics, short of how the computation is orchestrated. I think this is reasonable.
What was existing testing like? What have you done to improve it?: Not much: in fact removing this functionality simplifies the tests slightly.
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.): No NoSuchMethodError or similar things thrown from prod code
Has the safety of all log arguments been decided correctly?: N/A
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.): NoSuchMethodError for get thrown
If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?: Recall
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): N/A
Scale
Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: I don't think so, though see the concerns section
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)?: Not any more than now
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?: I doubt it.
Development Process
Where should we start reviewing?: TransactionKeyValueService?
If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?: N/A
Please tag any other people who should be aware of this PR:
@jeremyk-91
@sverma30
@raiju
General
This PR is part of a series: see the prototype https://github.com/palantir/atlasdb/pull/7000 or the internal RFC for what all of the pieces together are expected to look like. I have tried to separate this feature into reasonably sized components as otherwise it'd probably have a ~5k~ 10K delta or so!
Before this PR:
TransactionKeyValueServicesupports bothget()andgetAsync(). These have the same signature, exceptget()returns aMap<Cell, Value>whilegetAsync()returns aListenableFuture<Map<Cell, Value>>.After this PR:
==COMMIT_MSG==
TransactionKeyValueServiceonly supportsgetAsync(). Users ofget()should just callAtlasFuture.getUnchecked(...)or similar on the future returned fromgetAsync(). ==COMMIT_MSG==Priority: High P2 - nothing in particular but the workstream is a high priority.
Concerns / possible downsides (what feedback would you like?):
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?: YES - but this interface is only used by internal migration tooling and Atlas itself
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?: That
getAsync()andget()have the same semantics, short of how the computation is orchestrated. I think this is reasonable.What was existing testing like? What have you done to improve it?: Not much: in fact removing this functionality simplifies the tests slightly.
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.): No
NoSuchMethodErroror similar things thrown from prod codeHas the safety of all log arguments been decided correctly?: N/A
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.):
NoSuchMethodErrorforgetthrownIf this PR does not work as expected, how do I fix that state? Would rollback be straightforward?: Recall
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): N/A
Scale
Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: I don't think so, though see the concerns section
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)?: Not any more than now
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?: I doubt it.
Development Process
Where should we start reviewing?: TransactionKeyValueService?
If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?: N/A
Please tag any other people who should be aware of this PR: @jeremyk-91 @sverma30 @raiju