EventStore / EventStore-Client-Go

Go Client for Event Store version 20 and above.
Apache License 2.0
103 stars 25 forks source link

question: Persistent Subscription Docs vs. Code #176

Closed seanppayne closed 1 day ago

seanppayne commented 3 months ago
// PersistentAllSubscriptionOptions options for most of the persistent subscription requests.
type PersistentAllSubscriptionOptions struct {
    ...
    // Requires the request to be performed by the leader of the cluster.
    RequiresLeader bool
}

I am just curious, why is there an option for RequiresLeader when the docs state the following:

Persistent subscriptions only run on the Leader node. This means that more pressure will be put on the Leader node, and there is no option to balance the load to a follower like with a Catch-up subscription.

Is there a use case for this option or should I default to setting it to true?

YoEight commented 3 months ago

This is a fair question. As the documentation states it, it's not possible to run persistent subscription on anything than the leader node. All write operations and the subscribe don't need that option indeed. We should remove it from the code to avoid confusion.

seanppayne commented 3 months ago

I would be happy to try and make this change myself if you’re interested in having more contributors.

On Thu, Jun 6, 2024 at 12:48 AM Yo Eight @.***> wrote:

This is a fair question. As the documentation states it, it's not possible to run persistent subscription on anything than the leader node. All write operations and the subscribe don't need that option indeed. We should remove it from the code to avoid confusion.

— Reply to this email directly, view it on GitHub https://github.com/EventStore/EventStore-Client-Go/issues/176#issuecomment-2150401059, or unsubscribe https://github.com/notifications/unsubscribe-auth/BD3VQPC4KCYYE6LIM7ZXMX3ZF4XNVAVCNFSM6AAAAABIFDMLKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJQGQYDCMBVHE . You are receiving this because you authored the thread.Message ID: @.***>

YoEight commented 3 months ago

Please have a go if you want. Will be glad to approve your PR!

seanppayne commented 3 months ago

Cool, I will give it a shot this weekend.

seanppayne commented 3 months ago

@YoEight Two questions:

  1. I do not have permission to push code to a branch here if I clone the repo. Should I fork instead?
  2. I get the following error in make test. Do I need to adjust some env vars or do I need some credentials to access this image repository?
Failed to get image auth for docker.eventstore.com. Setting empty credentials for the image: docker.eventstore.com/eventstore-utils/testdata:21.10.0-focal. Error is:credentials not found in native keychain
YoEight commented 3 months ago

Hey @seanppayne

I do not have permission to push code to a branch here if I clone the repo. Should I fork instead?

Yes, you need to fork the repo first then open a pull request on the branch you used to write your patch

I get the following error in make test. Do I need to adjust some env vars or do I need some credentials to access this image repository?

You shouldn't have to anything more. Could it be you need to run docker while being root on your machine? I personally always run my docker containers rootless. In any case, don't stress over it, just write your patch, the CI will take care of the testing part.

Thanks for your contribution!