Skip to content

Multi-databases support for routing driver #587

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

Merged
merged 10 commits into from
Jul 3, 2019

Conversation

zhenlineo
Copy link
Contributor

@zhenlineo zhenlineo commented Apr 15, 2019

GetRoutingTable procedure now accepts an optional parameter database for fetching routing table for the specified database. To obtain routing table for default database, use database = null rather than database = ''.

GetRoutingTable procedure should be executed on system database as it is a DBMS procedure.

Basics:
RoutingTableRegistry keeps a concurrent map from database names to RoutingTableHandler.
RoutingTableHandler holds a RoutingTable (empty at creation) and handles removal of servers from routing table in case of connection errors.
Discovery provides discovery service. Given a routing table, it reads available routers and finds a connection from connection pool to perform discovery. If success, it updates the routing table and connection pool. If failed, it throws error.

RoutingTableRegistry, Discovery and ConnectionPool are shared resources by all threads in drivers. RoutingTableHandler and RoutingTable is (mostly) used by a single thread.

@zhenlineo zhenlineo force-pushed the 2.0-routing-table branch 4 times, most recently from a7a9a8d to a1ba224 Compare May 10, 2019 15:29
@zhenlineo zhenlineo marked this pull request as ready for review May 10, 2019 15:29
Zhen Li added 5 commits June 26, 2019 15:51
…abase.

As this makes the routing table map easier for server version lower than 4.0.
Otherwise, if we default to get routing table for system_db, we need to detect if it is server version lower than 4.0. If so, we can only get routing table for default_db, and then we need to reset the key in routing table map to be default_db.

TODO: Removing stale routing table from routing table map.
Also missing any integration tests for 4.0 routing driver.
@zhenlineo zhenlineo force-pushed the 2.0-routing-table branch 2 times, most recently from c567641 to 771e6d6 Compare June 27, 2019 15:23
@zhenlineo zhenlineo force-pushed the 2.0-routing-table branch from 771e6d6 to 63d6f93 Compare June 27, 2019 15:53
@zhenlineo zhenlineo force-pushed the 2.0-routing-table branch from 0de7d22 to d09054a Compare June 28, 2019 13:54
@zhenlineo zhenlineo requested a review from ali-ince June 28, 2019 14:26
…getRoutingTable`, as this procedure is not only used by cluster but also single instance.
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.

@zhenlineo Looks good to me. I've made small comments mostly with naming suggestions.

{
if ( serverVersion.greaterThanOrEqual( v3_2_0 ) )
if ( serverVersion.greaterThanOrEqual( v4_0_0 ))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should consider moving to compare protocol versions instead of server versions over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking we cannot test bolt version as the procedures are not part of bolt protocol.

Fixed a flaky test due to misused timeout value
@zhenlineo zhenlineo force-pushed the 2.0-routing-table branch from 800d376 to b6d1c2a Compare July 2, 2019 20:34
@zhenlineo
Copy link
Contributor Author

I lost one comment. I left a TODO to make STALE_ROUTING_TABLE_PURGE_TIMEOUT a configuration option, which will comes after all other commits to avoid merge conflicts.

@zhenlineo zhenlineo merged commit 85964dd into neo4j:2.0 Jul 3, 2019
@zhenlineo zhenlineo deleted the 2.0-routing-table branch July 3, 2019 08:15
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