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

Adding tx with built-in retries support for RxSession #582

Merged
merged 7 commits into from
Apr 15, 2019

Conversation

zhenlineo
Copy link
Contributor

@zhenlineo zhenlineo commented Mar 22, 2019

Implemented RxSession#readTransaction and RxSession#writeTransaction using Flux#retryWhen.
Extracted NetworkSession to provide basic runAsync and runRx methods, allowing other session implementations (such as AsyncSession, RxSession) build on top of it.

@zhenlineo zhenlineo force-pushed the 2.0-with-retries branch 4 times, most recently from 968dba7 to dbce261 Compare April 5, 2019 14:43
@zhenlineo zhenlineo marked this pull request as ready for review April 5, 2019 14:47
Zhen Li added 3 commits April 10, 2019 12:02
By default, Project Reactor schedules delayed operations on a parallel scheduler, which is created lazily when needed.
We can avoid creating such an extra resource by schedule the delayed work on our existing netty event loop thread pool.
This behaviour is exactly the same as our existing async retries.

Added tests to verify that the Parallel scheduler is not created.
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.

I had one naming suggestion against the internal classes.

import org.neo4j.driver.internal.cursor.RxStatementResultCursor;
import org.neo4j.driver.internal.spi.Connection;

public interface ExplicitTransaction
Copy link
Contributor

Choose a reason for hiding this comment

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

I slightly got confused with the new naming scheme. Since both NetworkSession and ExplicitTransaction are core building blocks for the rest of the surfaced API, what do you think about renaming ExplicitTransaction to NetworkTransaction - just to form a relationship between the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name NetworkTransaction dose not fits the Transaction very much. Will it be more clear if I remove the interfaces and directly call the implementations NetworkSession and ExplicitTransaction?

Then all sessions will be built on top of NetworkSession and all transactions are built on top of ExplicitTransaction.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I think that would help.

…ion`.

And put implementation directly instead.
@zhenlineo zhenlineo merged commit 488e33d into neo4j:2.0 Apr 15, 2019
@zhenlineo zhenlineo deleted the 2.0-with-retries branch April 15, 2019 09:29
@zhenlineo zhenlineo changed the title Adding Session run tx with retries Adding tx with built-in retries support for RxSession May 15, 2019
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.

2 participants