Skip to content
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

Improvements for StatementResultCursor API #405

Merged
merged 8 commits into from
Sep 22, 2017

Conversation

lutovich
Copy link
Contributor

Based on #404.

Main changes:

  • replaced #fetchAsync() + #current() with single #nextAsync() to simplify handling of futures
  • added #forEachAsync()
  • added #listAsync()

This is a test-only implementation of `RetryLogic` interface that
retries up to a specified amount of times. It previously had a
separate implementation which was basically a simplified version
of `ExponentialBackoffRetryLogic`. It is problematic for following
`retryAsync` functionality because implementation will not be that
simple. This commit makes `FixedRetryLogic` extend
`ExponentialBackoffRetryLogic` and override just a single method.
So that they can share complicated retrying code.
This commit makes retry logic able to retry work that executes async
operations and return a `Future`. It's a first step towards supporting
async transaction functions.

Also renamed future combinator methods to better represent their intent.
Via `Session#readTransactionAsync()` and `#writeTransactionAsync()`.
Both accept a single parameter - async function that takes a
transaction and returns a `Response`. When function returns a failed
response or throws exception it will be retried. Retries only happen
on non-fatal & transient errors.

API is not fully complete yet - there is no way to create a `Result`
object so it'll be inconvenient to use. Helpers to create some sort
of "settable" response will come in subsequent commits.
Given work might fail by throwing exception or by returning a failed
future. This commit makes retries gracefully handle exceptions.
Previously retrying code would only log an exception and hang.
Transaction should be marked as failed when queries fail. It was
previously only marked when PULL_ALL failed. This commit makes RUN
handler notify transaction on failure.
Commit replaces `Response<Boolean> #fetchAsync()` + `Record #current()`
in `StatementResultCursor` with single `Response<Record> #nextAsync()`.
This simplifies API and makes it more convenient to consume records via
future chaining and callbacks. `Response` is already a container for
record, it can't contain anything else. So it makes sense to return
`null` as an end of records stream marker and not use separate method
for this.

Also added `#peekAsync()` method.
Method allows processing of every incoming record in an async fashion.
It returns a future which signals about processing completion. It can
either be failed or completed with `null`.
Method allows async retrieval of all records as list.
Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

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

LGTM!

@lutovich lutovich merged commit 192d33b into neo4j:1.5-async Sep 22, 2017
@lutovich lutovich deleted the 1.5-async-result-cursor-api branch September 22, 2017 12:00
@zhenlineo
Copy link
Contributor

zhenlineo commented Sep 25, 2017

A few commits:

  1. How do we expect a user to use cursor.nextAsync()? It seems to me that a user should never use cursor.nextAsync(), instead they should probably use #singleAsync() (missing), #forEachAsync, or #listAsync.
  2. If we replaced #fetchAsync() + #current() with single #nextAsync(), then maybe we should go over with .NET and JS API to see if we should uniform or differ there?
  3. I thought #forEachAsync would return summary :P
  4. Shall we also have public Response<List<Record>> listAsync(final Consumer<Record> action)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants