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

Async get on transactions #4301

Merged
merged 16 commits into from
Oct 16, 2019
Merged

Async get on transactions #4301

merged 16 commits into from
Oct 16, 2019

Conversation

OStevan
Copy link
Contributor

@OStevan OStevan commented Oct 14, 2019

Goals (and why):
Add async get to Transaction API. Expose the asynchronous API from the KVS to the transactional layer.

Implementation Description (bullets):

  • straightforward propagation of getAsync from the KVS layer to the Transaction

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

  • tests are now parametrized to run with delegating get once to synchronous and once to asynchronous get
  • parameterized tests should cover all the cases

Concerns (what feedback would you like?):

  • if all transaction types have a correct implementation of the getAsync
  • should the code in tests be refactored to be more centralized (with respect to delegates)

Where should we start reviewing?:

  • Transaction

Priority (whenever / two weeks / yesterday):
tomorrow

@changelog-app
Copy link

changelog-app bot commented Oct 14, 2019 •
edited by OStevan
Loading

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Transactions can now support asynchronous get operations. This has been implemented for all transaction types. Feature is designed for direct uses of transactions.

Check the box to generate changelog(s)

  • Generate changelog entry

@OStevan OStevan force-pushed the so/async-get-transaction branch from ab6acaa to 6c77941 Compare October 15, 2019 15:38
@OStevan OStevan changed the base branch from develop to so/snapshot-get-refactor October 15, 2019 15:39
@OStevan OStevan force-pushed the so/async-get-transaction branch 2 times, most recently from da4451c to 978daf8 Compare October 15, 2019 16:08
@OStevan OStevan changed the base branch from so/snapshot-get-refactor to develop October 15, 2019 16:09
@OStevan OStevan force-pushed the so/async-get-transaction branch from 978daf8 to c224fc3 Compare October 15, 2019 16:14
jeremyk-91
jeremyk-91 reviewed Oct 15, 2019
View reviewed changes
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.

Broadly looks fine. There are some nuanced semantics in the tests that I have some comments on - might be worth a second look.

atlasdb-api/src/main/java/com/palantir/atlasdb/transaction/api/Transaction.java Outdated Show resolved Hide resolved
.../palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java Outdated Show resolved Hide resolved
...st/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java Show resolved Hide resolved
atlasdb-client/src/main/java/com/palantir/atlasdb/logging/KvsProfilingLogger.java Show resolved Hide resolved
...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java Show resolved Hide resolved
atlasdb-tests-shared/src/main/java/com/palantir/atlasdb/transaction/impl/GetAsyncDelegate.java Show resolved Hide resolved
...tests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/CachingTransactionTest.java Outdated Show resolved Hide resolved
...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
felixdesouza
felixdesouza suggested changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@felixdesouza felixdesouza left a comment

Choose a reason for hiding this comment

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

some stuff to fix up in tests, and some weird formatting to undo

.../palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java Show resolved Hide resolved
.../palantir/atlasdb/keyvalue/cassandra/CassandraKeyValueServiceTransactionIntegrationTest.java Outdated Show resolved Hide resolved
...st/java/com/palantir/atlasdb/keyvalue/cassandra/CassandraKvsSerializableTransactionTest.java Outdated Show resolved Hide resolved
...impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SerializableTransaction.java Outdated Show resolved Hide resolved
...sdb-impl-shared/src/main/java/com/palantir/atlasdb/transaction/impl/SnapshotTransaction.java Outdated
validatePreCommitRequirementsOnReadIfNecessary(tableRef, getStartTimestamp());
return removeEmptyColumns(result, tableRef);
},
MoreExecutors.directExecutor());
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny bit concerned with the amount of work being done here, especially with the directExecutor which does have its warnings.

...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
...ests-shared/src/test/java/com/palantir/atlasdb/transaction/impl/SnapshotTransactionTest.java Outdated Show resolved Hide resolved
@OStevan OStevan force-pushed the so/async-get-transaction branch from 34866b0 to 0b1374d Compare October 16, 2019 09:52
felixdesouza
felixdesouza approved these changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@felixdesouza felixdesouza left a comment

Choose a reason for hiding this comment

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

Looks good!

@OStevan OStevan merged commit c8d14ad into develop Oct 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the so/async-get-transaction branch October 16, 2019 12:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gmaretic gmaretic gmaretic left review comments

@jeremyk-91 jeremyk-91 jeremyk-91 left review comments

@jkozlowski jkozlowski jkozlowski left review comments

@felixdesouza felixdesouza felixdesouza approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@OStevan @jkozlowski @felixdesouza @jeremyk-91 @gmaretic