-
Notifications
You must be signed in to change notification settings - Fork 63
Allow creation of Transaction/AsyncTransaction from TxnContext #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow creation of Transaction/AsyncTransaction from TxnContext #149
Conversation
|
@gitlw @deepakjois @mangalaman93 please let me know what you think about this, it is tested to give me what I need for the Spark Dgraph connector (see linked PR) |
|
Hi @EnricoMi, Thanks for the PR. I have seen your post in discuss, I will take a look at it by the end of the week. |
|
Thanks, that's appreciated! |
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.
It is really a nice idea to expose TxnContext to users.
Thank you @EnricoMi for this nice PR.
Cheers!
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @EnricoMi)
src/main/java/io/dgraph/AsyncTransaction.java, line 61 at r1 (raw file):
Quoted 10 lines of code…
AsyncTransaction(DgraphAsyncClient client, DgraphStub stub, TxnContext context) { this(client, stub); this.context = context; } AsyncTransaction( DgraphAsyncClient client, DgraphStub stub, TxnContext context, final boolean readOnly) { this(client, stub, context); this.context = context; this.readOnly = readOnly; }
Just a minor thing, but I feel like these two constructors should go after this one: AsyncTransaction(DgraphAsyncClient client, DgraphStub stub, final boolean readOnly)
src/test/java/io/dgraph/DgraphAsyncClientTest.java, line 112 at r1 (raw file):
).get()
nitpicking, but we prefer join over get.
src/test/java/io/dgraph/DgraphAsyncClientTest.java, line 112 at r1 (raw file):
uid(0x0)
Although, this works currently. But, just to clarify, 0x0 is not a uid that is ever allocated. Uids always start from 0x1. So, maybe use 0x1 in all the test queries.
src/test/java/io/dgraph/DgraphAsyncClientTest.java, line 121 at r1 (raw file):
).get();
join over get again.
src/test/java/io/dgraph/DgraphAsyncClientTest.java, line 121 at r1 (raw file):
0x0
0x1
src/test/java/io/dgraph/DgraphClientTest.java, line 107 at r1 (raw file):
0x0
0x1
src/test/java/io/dgraph/DgraphClientTest.java, line 116 at r1 (raw file):
0x0
0x1
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.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur and @EnricoMi)
src/test/java/io/dgraph/DgraphAsyncClientTest.java, line 109 at r2 (raw file):
@Test public void testNewTransactionFromContext() throws Exception {
I guess, you won't need throws Exception anymore as all the exceptions returned by join are runtime exceptions.
src/test/java/io/dgraph/DgraphAsyncClientTest.java, line 118 at r2 (raw file):
@Test public void testNewReadOnlyTransactionFromContext() throws Exception {
throws Exception can be removed.
7af1462 to
1bbae4b
Compare
|
Thanks for merging this! Any estimate on when this will be part of a release? Or can you provide me a snapshot version? |
|
Umm..., we are gonna do a major release for dgraph by the end of July, and I think we will also release a new version of this client in sync with the dgraph release, as it has got some updates. But, let me confirm, and update you on this. |
This PR adds support for creating a new transaction from
DgraphClientandDgraphAsyncClientusing an existingTxnContext.This change is