wolf4ood / gremlin-rs

Gremlin Rust
Apache License 2.0
108 stars 30 forks source link

MergeV, MergeE, & Option Steps #214

Closed criminosis closed 1 month ago

criminosis commented 4 months ago

This PR does the following:

criminosis commented 4 months ago

@wolf4ood didn't realize this got over 1k lines πŸ˜… , let me know what you think. Happy to make changes πŸ‘ .

criminosis commented 4 months ago

@wolf4ood Added a couple more things today

criminosis commented 3 months ago

@wolf4ood I think I've put in some missing logic in commit https://github.com/wolf4ood/gremlin-rs/pull/214/commits/f8f5fec156d40a0513d64c9680f52f8ab0420e80 if the websocket peer closes the async connection. Basically mirrored sync version over here: https://github.com/wolf4ood/gremlin-rs/blob/d7ce0e42574458d7d5fef7c787684f3b74b88699/gremlin-client/src/connection.rs#L289-L306

I've been having issues where a peer would reset after a long duration of writes but seemed like I kept getting the same broken connection for my retries. Figured I'd tuck it into this PR since it was still open.

wolf4ood commented 1 month ago

Hey @criminosis

thanks for the hard work let me know when the PR is ready for review i will take a look :)

criminosis commented 1 month ago

Yeah I started tinkering with other stuff πŸ˜… .

Lemme back out the exploratory logging & and the experimental connection muxing.

Are you okay with the rest of the stuff going in as a single PR? Might take a while to splice everything else out at this point πŸ€”

wolf4ood commented 1 month ago

@criminosis

if it's ready for review as it is i would keep it in a single PR, I might have some time in the next days to go through it

criminosis commented 1 month ago

@wolf4ood think I've got it tuned back to being in a ready to merge state πŸ‘ . I updated the OP with I think the additional items since I stopped updating it.

Whenever you're able to review it and it gets merged mind cutting a release please?

criminosis commented 1 month ago

Hey @wolf4ood do you think you'd still be able to review this in the near future? I'm working on implementing the GraphBinary protocol into this library (the follow-up PR I mentioned previously).

It's going to involve moving "load bearing walls" to make space for it though. A handful of places I've noticed seem to assume JSON based serialization & deserialization, so it's still gonna take a few days but it's eventually gonna be stuck behind this one merging.

It may end up being best to do a PR that tries to make that space first, and then put up the PR that actually adds the GraphBinary protocol.

I'm striving to hide everything behind the serializer/deserializer options configured in the client, but I think it's going to be unavoidable as a breaking change if people were explicitly setting their serializer/deserializer. I'll leave the default of GraphSONV3 though. Because at a bare minimum I'd like to rename that enum to a more generic sounding name. Otherwise it looks like:

pub enum GraphSON {
    V2,
    V3,
   GraphBinaryV1
}

Which doesn't make much sense. Tentatively moving them into:

pub enum IoProtocol {
    GraphSONV2,
    GraphSONV3,
    GraphBinaryV1
}