Skip to content
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

SBT-9849 EventStore.Client.Grpc.Streams #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JamesMcCaig
Copy link

Upgrade from .netStandard 2.0 to .Net6
Change EventStoreEventTools to use new methods in the GRPC EventStore Client
Include Newtonsoft newest Version
Change EventStoreConnectionbuilder to EventStoreClientBuilder
Include a EventStoreClient GetEventStoreClient function which returns a EventStoreClient given a connectionstring
Include new Test functions for ToEventData

…lient.Grpc 22.0.0

Include Newtonsoft upgrade
Include .Net6.0 Upgrade to solution and project (upgraded using auto upgrade tool)
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Pull Request Review Checklist:

  • Variable have been given sensible, meaningful names.
  • LINQ to SQL datacontexts must always be initialised with an explicit connection string drawn from a config file.
  • Code is DRY (Dont Repeat Yourself). (But make sure where this is applied, the two things are actually the same!)
  • No values have been hard-coded where they should be drawn from a config file.
  • Comments are present where appropriate, and provide useful information not just stating what the code is obviously doing.
  • Error handling is performed appropriately where things might fail.
  • Logging is present, useful, and uses log levels appropriately.
  • If new app config values are referenced in the code, a placeholder must be committed in the config file.
  • If new app config values are referenced in the code, they must be added to octopus before approving this PR.
  • For projects with snapshotting, any new fields added to viewmodels must declare datamembers appropriately.
  • For projects with snapshotting, any new viewmodels should be added to the snapshot load/save code in the bootstrapper.

Copy link
Member

@jbradford jbradford left a comment

Choose a reason for hiding this comment

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

A question on this, not sure if was discussed elsewhere.

/// <returns>EventStoreClient.</returns>
public static EventStoreClient GetEventStoreClient(string connectionString)
{
var settings = EventStoreClientSettings.Create(connectionString);
Copy link
Member

Choose a reason for hiding this comment

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

Is this definitely all we need to specify compared to the old version? Seems like previously we were setting a gossipTimeout, is that not something we still need to support or is there a different way of setting it now?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed with @jbradford and @JamesMcCaig in Teams. Neither of us have been able to find references to this in the documentation, which suggests that the approach that we have taken here is sufficient

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.

None yet

3 participants