search

cleishm / libneo4j-client

neo4j-client -- Neo4j Command Line Interface (CLI)
https://neo4j-client.net
Apache License 2.0
155 stars 38 forks source link

Neo4j 4 compatibility? #49

Open johannessen opened 4 years ago

johannessen commented 4 years ago

Trying to use libneo4j-client 2.2.0 with the first release candidate for Neo4j 4, which was published a week ago, only yields a -14 error (NEO4J_PROTOCOL_NEGOTIATION_FAILED). Looks like support for Bolt versions 1 and 2 was removed from Neo4j 4 with https://github.com/neo4j/neo4j/commit/9ff0780b3458fd2f6abcbe90d61a513c3a30e956.

Is there any information available about future compatibility of libneo4j-client with Neo4j 4?

cleishm commented 4 years ago

It does seem that they have removed support for version 1 of the protocol, without publishing any details on the new versions.

This requires someone to spend a little time going through the code for Neo4j and determining what they have changed in the new protocol versions, then implementing those changes in this project. Many of the new types are already available (as mentioned in #36).

Assuming their change is not extensive, it should be a fairly straight-forward task, but it does require someone to do it.

johannessen commented 4 years ago

Right. Thanks for clarifying.

johannessen commented 4 years ago

For what it may be worth, they say in https://github.com/neo4j/neo4j/issues/12361#issuecomment-570238798 that they do intend to get around to add protocol documentation at some point…

davisford commented 4 years ago

@cleishm I might be willing to try to help with @johannessen if you can give some pointers

We use the client now as part of our build/deploy process to run migration / index creation with cypher and that is currently broken since we upgraded to neo4j 4

cleishm commented 4 years ago

Happy to give pointers - I just don't have so much time to do it myself. Could arrange a call if you like - send me an email.

davisford commented 4 years ago

Just a follow-up -- @johannessen I had a call with Chris today and he walked me through several pointers. I need to study it a bit more. Looks like protocol v3 might be lower hanging fruit -- fewer changes.

If it looks like adding support for protocol v3 won't be a herculean effort, I may take a stab at it. Let me know if you want to collaborate.

johannessen commented 4 years ago

Unfortunately I am not currently in a position to contribute to this project myself.

cleishm commented 4 years ago

@davisford (and others) - a quick question for you. I'm finishing off support for all the additional types that were added in neo4j's implementation of later protocol versions, which should be the hardest part of getting the new protocol versions working.

A have a question of style/ease of use, and would like your thoughts on.

Some of the new types require significantly more data to be carried with each value (e.g. the point type requires up to 3 doubles plus the srid). This is more than the 128 bits (16 bytes) that the neo4j_value_t currently uses.

So I either make the user provide additional storage when creating these more esoteric types, e.g.: https://github.com/cleishm/libneo4j-client/blob/07c0876b33ab0d7cd7dc96f055aa0b85446e44a5/lib/src/neo4j-client.h.in#L1302-L1303 https://github.com/cleishm/libneo4j-client/blob/07c0876b33ab0d7cd7dc96f055aa0b85446e44a5/lib/src/neo4j-client.h.in#L1312-L1313

Or, I expand the size of neo4j_value_t - at least doubling it to 256 bits (32 bytes) (but more likely to 320 bits) - which will affect the size of every value represented, including basic values such as integers, etc. This will increase the overall memory consumption by the driver when handling results streams.

Thoughts? I've currently been implementing using the former approach, but I'm concerned that this managing of additional storage creates an overly troublesome burden on users of the API, vs not requiring it. However, this "management of additional storage" does already exist implicitly when creating a neo4j string value: https://github.com/cleishm/libneo4j-client/blob/07c0876b33ab0d7cd7dc96f055aa0b85446e44a5/lib/src/neo4j-client.h.in#L787-L788

So perhaps it's not unreasonable to extend that elsewhere.

davisford commented 4 years ago

@cleishm sorry for the lag in response -- been swamped myself. from my perspective as a user -- it doesn't matter to me...lol, selfishly whatever would be easier for you. my usage of this client is in an init container in our kubernetes cluster. every time we deploy our new gRPC API on top of neo4j, we run a .cypher file using your client to ensure the indices we want are up to date. it's something that doesn't happen often, and runs quickly enough that memory is of little concern especially for this C program.

Not sure if that provides a great deal of insight for you. I'm going to try to take a deeper look at the code again this weekend. It's always a matter of time, though...which I also don't have a ton of, but I'd hate to see this project abandoned. It's a really great lib/tool.

EDIT -- also wonder how seabolt lib is doing it? have you looked at their type defs?

EDIT EDIT -- if you're already requiring client of the lib to manage their own memory with the string type, I'd stick with the same pattern for the new, larger types like point which won't fit in your neo4j_value_t. I don't see a big problem with that, honestly.

majensen commented 4 years ago

Hey guys - I'm pretty interested in seeing this happen (@johannessen and I have a Perl wrapper for bolt based on this great library). I'm willing to help (tho' I don't have many tuits either) - let me know. (@cleishm re:neo_value_t, I agree with @davisford, plus it would be easier to just fold in the new types to our wrapper if I don't have to do anything special for them. :D )

FrankR85 commented 4 years ago

Are there any news for this issue? We are trying to use the neo4j-client 2.2.0 with Neo4J 4, but are getting 'error: Could not agree on a protocol version'.

johannessen commented 3 years ago

I see there’s protocol documentation at https://7687.org/ now. ~(https://github.com/neo4j/neo4j/issues/12361 is still open though.)~

gz15028 commented 3 years ago

Is this issue still being followed? Thanks!

cleishm commented 3 years ago

Followed? Yes! Would love to see people interested in this put time into implementing it :)

gz15028 commented 3 years ago

Interested? Yes! Just tried with version 3.5.23 + GDS 1.1.6 and it worked fine :-). The C/C++ driver is useful to many people I guess. I was intially expecting one on win/mingw, but no, then I swithed to Ubuntu, it showed protocol problem, and then i degraded to version 3 and finally! Unfortunately, i dont think i am already at a level to help in implementing :-(

majensen commented 3 years ago

@cleishm I've started grokking the protocol and the code. I think it is doable and I will be taking a stab it on https://github.com/majensen/libneo4j-client/tree/feat-bolt-3.0. Can't guarantee any timeline, but I'm feeling fairly confident.

cleishm commented 3 years ago

Amazing. I'm also happy to get on a zoom call and walk through the code for a few hours together with anyone taking this on. Just let me know.

johannessen commented 3 years ago

@majensen Sounds great!

Hey, I'm not sure if you know about Neo4j's new-ish Slack channel #community-driver-authors? Several key people from Neo4j Engineering are on that channel, ready to quickly answer any specific questions about Bolt etc. that implementors might have. @fbiville should be able to add you to that channel if you’d like.

majensen commented 3 years ago

Progress report: Things are actually almost working for Bolt 3, which is the big leap (4 is a matter of a couple of details). The main functionality added there is the explicit transaction capabilities - begin, commit, run in transaction, and rollback. Along with updating the version negotiation and adding new messages and errors and such, I've added a transaction object and API functions for these tasks. A new test/check_transaction.c file adds some tests against Chris' mock server that all pass (with "make check").

Am now pointing it at a real server and fixing the details. There are slight changes in the structure (the "argv/argc") in 3+ for the metadata that accompany some of the messages. Taking care of these enables the server to successfully respond. The code is aware of the bolt version and branches appropriately so the client should remain backward compatible with Bolt 1/2.

majensen commented 3 years ago

Ok, still some work to do, but I have merged it all into master on https://github.com/majensen/libneo4j-client.

majensen commented 3 years ago

Guinea pigs are welcome to try out https://github.com/majensen/libneo4j-client on Neo4j v3. I have added :begin / :commit / :rollback commands that work to the shell.

It won't work on Neo4j v4.0 yet. That is the next milestone.

johannessen commented 3 years ago

Nice work so far!

I’ve been getting a lot of “Transaction timed out” errors when running https://github.com/majensen/libneo4j-client/commit/9b5f358d5eeb0a1378515f46469967f47b8cadd0. My server doesn’t have such a timeout (dbms.transaction.timeout is unset, and transactions do seem to remain open indefinitely on Bolt v1).

I think that when the user doesn’t specify a timeout, tx_timeout should not be provided in the Bolt BEGIN message. Not doing so should result in the use of the timeout configured in the server (dbms.transaction.timeout in neo4j.conf). By default, this config option is unset, which disables the timeout feature entirely for Bolt.

majensen commented 3 years ago

@johannessen great call. Could you make an issue in the fork and tag me in the body?

majensen commented 3 years ago

@johannessen @foobargeez I believe we have liftoff for Bolt V4.0. Please try master branch on https://github.com/majensen/libneo4j-client. The expected features now are

cleishm commented 3 years ago

This is great. I'll try to find some time to review the code also and give some feedback. Do you want to open a merge PR sometime?

majensen commented 3 years ago

Will do - thanks!

majensen commented 3 years ago

Many issues now cleared up at PR #57 thanks to @johannessen testing. I just pushed a new version of majensen/perlbolt that uses it; on CPAN as Neo4j:: Bolt v0.40.

m-pi commented 3 years ago

Have you doing progress guys? I need this ;-) as a sun!

majensen commented 3 years ago

@m-pi try the fork at https://github.com/majensen/libneo4j-client - it should be working. It hasn't been pulled into the main repo yet - waiting for @cleishm to review.