Skip to content

Add bookmarks support #238

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 6 commits into from
Apr 3, 2025
Merged

Add bookmarks support #238

merged 6 commits into from
Apr 3, 2025

Conversation

madchicken
Copy link
Contributor

@madchicken madchicken commented Mar 30, 2025

This PR aims to add support for bookmarks in transactions. It also collects bookmarks used in a routed manager and sends them when requesting a new routing table. The list of collected bookmarks is cleared up every time a READ operation returns no bookmarks (that should mean the state of the cluster is consistent).
The bookmarks are sent in the BEGIN message and retrieved from the COMMIT message.

Doubts

I read somewhere that the command HELLO can also contain bookmarks, but the documentation does not say a thing about it.

Bonus: While implementing this feature, I found a problem when getting a connection in a routed environment, caused by a synchronization problem between the manager and the thread fetching the routing table.

Changes

Transaction Handling Updates:

  • Added a new Begin request type with associated metadata and builder pattern for constructing instances. (lib/src/bolt/request/begin.rs)
  • Modified the Commit request type to include a CommitResponse that handles bookmarks. (lib/src/bolt/request/commit.rs) [1] [2] [3]
  • Updated the Graph struct to support starting transactions with bookmarks and to handle bookmarks in query execution results. (lib/src/graph.rs) [1] [2] [3] [4] [5] [6]

Routing Mechanism Updates:

  • Added a new Begin request type to the bolt/request/mod.rs module. (lib/src/bolt/request/mod.rs) [1] [2]
  • Modified the Route request type to remove lifetime parameters and updated serialization logic. (lib/src/bolt/request/route.rs) [1] [2] [3] [4] [5] [6]

Miscellaneous:

  • Introduced a Bookmark trait to standardize bookmark handling across different response types. (lib/src/bookmarks.rs)
  • Updated the Connection struct to support the new Route request type without lifetime parameters. (lib/src/connection.rs)

@madchicken
Copy link
Contributor Author

I see tests fail since they run without the unstable-bolt-protocol-impl-v2 feature (sorry, I didn't think about that). Do you want me to remove them?

@knutwalker
Copy link
Collaborator

You can run the test only when the feature is enabled, e.g.

#![cfg(feature = "unstable-result-summary")]

@madchicken
Copy link
Contributor Author

I fixed the issue, but I had to run the new integration test under the feature unstable-bolt-protocol-impl-v2 while I see you created a specific feature for the summary (for example). Do you want a new feature for bookmarks?

@knutwalker
Copy link
Collaborator

No, it's fine. I think now I overdid it with the various unstable subfeatures, they should probably collapsed into the unstable-v1 feature instead of new ones being added.

Copy link
Collaborator

@knutwalker knutwalker left a comment

Choose a reason for hiding this comment

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

Nice, this looks good.
One change it would like to make is to not have a signature change on the commit method when the unstable feature is not enabled.

@madchicken madchicken requested a review from knutwalker April 2, 2025 16:50
Copy link
Collaborator

@knutwalker knutwalker left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@knutwalker
Copy link
Collaborator

Thanks for the PR, this looks great!

@knutwalker knutwalker merged commit 1f146ac into neo4j-labs:main Apr 3, 2025
11 checks passed
@madchicken madchicken deleted the bookmarks branch April 7, 2025 09:54
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