search

EventStore / EventStoreDB-Client-Java

Official Asynchronous Java 8+ Client Library for EventStoreDB 20.6+
https://eventstore.com
Apache License 2.0
63 stars 20 forks source link

Improve handling of errors on subscribing to streams and persistent subscriptions. #97

Closed dpasek-senacor closed 3 years ago

dpasek-senacor commented 3 years ago

Improve handling of errors on subscribing to streams and persistent subscrptions.

Currently gRPC excpetions on subscribe do not complete the future which will result in infinite runtime of future. A client waiting for the subscription will block unnecessarily and run into timeout exception instead of the exception thrown by subscribing.

This pull request changes the behavior, that exceptions occuring before the subscription being confirmed will complete the future exceptionally.

dpasek-senacor commented 3 years ago

@YoEight I will try. I currently have some issues running the integrations tests locally with the test container. It does not fetch/start the Docker images. I didn't have time to look into it.

YoEight commented 3 years ago

@dpasek-senacor Yeah you have to pull the image first.

dpasek-senacor commented 3 years ago

Hi @YoEight I have added a test case for the subscribe to persistent subscription use case. I failed at applying the same test case for non-persistent subscriptions as I could not invoke an error on subscribe: you can currently subscribe to unknown streams etc. Is there a way to (easily) invoke an error on subscribing for non-persistent subscriptions?

YoEight commented 3 years ago

@dpasek-senacor The best you can do in this case is to shutdown the database.

dpasek-senacor commented 3 years ago

@YoEight

The best you can do in this case is to shutdown the database.

Thanks! Works like a charm... :-)

pvanbuijtene commented 3 years ago

Looks good @dpasek-senacor, thanks for your contribution 👍 Would you mind rebasing and squash everything into a single commit?

dpasek-senacor commented 3 years ago

@pvanbuijtene Sorry, but I have problems squashing/rebasing as I did a merge from trunk (to resolve conflicts) and now squashing on my side is no longer possible. Any options to avoid squashing on my side?

Should it possible for you to merge by squashing? See: https://docs.github.com/en/github/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/merging-a-pull-request#merging-a-pull-request

Sorry for being new to this "contribution game" and the github worklflows. 😄

pvanbuijtene commented 3 years ago

No worries, I'll see how I can help to get your changes merged.

pvanbuijtene commented 3 years ago

@dpasek-senacor I collected some instructions on how to update this pull request to a single commit, we do this for keeping a clean commit history. I hope they help, you can add me pvanbuijtene as a contributor to your repository if you want me to do it.

Setup remotes: Before starting it's good to understand how my remotes are setup, I use origin for my own repository and upstream for the EventStore repository. This is how it looks like:

$ git remote -v
origin  git@github.com:pvanbuijtene/EventStoreDB-Client-Java.git (fetch)
origin  git@github.com:pvanbuijtene/EventStoreDB-Client-Java.git (push)
upstream        git@github.com:EventStore/EventStoreDB-Client-Java.git (fetch)
upstream        git@github.com:EventStore/EventStoreDB-Client-Java.git (push)

You can use any name you want, but below I will refer to origin and upstream.

Remove merge commit and squash:

  1. Configure your preferred text editor, instructions can be found here, and checkout the branch if not the case yet.
  2. Start interactive rebasing of the 4 commits on the branch: git rebase --interactive head~4
    1. Pick the first commit (767d1d2edca51576b3fc9fb161f7c08bd011e8dd) and fixup the 2nd (c896d4292654fd692d5afd3ff37178cb9bd03ede) & 3rd (b590d43e3536b5f39a55362645d832ac960e58fe) commits.
    2. Drop all non-relevant commits.
    3. After this it should look similar like this: 
      
pick 767d1d2 Improve handling...
fixup c896d42 Add test case...
fixup b590d43 Simplify tests...
drop e7a4a6d Add type information...
drop 490e30f Add equals() and...
drop 56fa60a Add equals() hashCode()...
drop ba7f421 Avoid calling...

... comments are ignored`



4. Save your changes and close the file.
4. This step is not needed, but always nice to have a backup. Force push the changes to your remote branch in case you need to roll back for some reason: `git push --force origin head`

**Rebase latest changes:**
1. Get all latest changes: `git fetch --all`
2. Rebase upstream trunk on your local branch: `git rebase upstream/trunk`
3. Resolve the conflicts in you editor.
4. Stage the changes: `git add *`
5. Continue the rebasing process: `git rebase --continue`, this will popup your editor for changing the commit message, change it or keep current and close the file.
7. Push the new commit to your remote branch: `git push --force origin head`

If you have any question, please let me know.
dpasek-senacor commented 3 years ago

@pvanbuijtene Thanks for your help. I think I got it right now. I just created a new local branch from trunk, cherry picked everything (my way of rebasing. ;-) ) , fixing the conflicts, squashed the local branch and force pushed it to the branch origin holding the PR.

pvanbuijtene commented 3 years ago

Great, thanks for the effort 👍