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.

Sign up for GitHub

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

Jump to bottom

Update persistent subscriptions to minimum viable tier #180

Merged
merged 1 commit into from
Dec 27, 2022
Merged

Update persistent subscriptions to minimum viable tier #180

merged 1 commit into from
Dec 27, 2022

Conversation

pvanbuijtene
Copy link
Contributor

@pvanbuijtene pvanbuijtene commented Jan 20, 2022 •
edited
Loading

Added: Support GetInfo() over gRPC for returning details of a persistent subscription
Added: Support ReplayParked() over gRPC for replaying parked messages
Added: Support List() over gRPC for listing persistent subscriptions
Added: Support RestartSubsystem() over gRPC for restarting the persistent subscription subsystem

Closes #147
Closes #185

@pvanbuijtene pvanbuijtene marked this pull request as draft January 20, 2022 10:52
@hayley-jean hayley-jean marked this pull request as ready for review January 24, 2022 14:06
oskardudycz
oskardudycz reviewed Jan 26, 2022
View reviewed changes
Copy link
Contributor

@oskardudycz oskardudycz left a comment

Choose a reason for hiding this comment

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

@pvanbuijtene, changes look good, besides the missing fallbacks to HTTP API and/or adding restart subsystem that, as I recall, is not working yet correctly in each case.

timothycoleman
timothycoleman requested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

would be worth rebasing, the names of some methods have changed in master so they'll need updating even though there aren't (many) merge conflicts

@pvanbuijtene pvanbuijtene force-pushed the viable-perst branch 4 times, most recently from bcee6a4 to 2b8e0a5 Compare February 3, 2022 23:43
@pvanbuijtene pvanbuijtene force-pushed the viable-perst branch 2 times, most recently from f6b5084 to b831653 Compare February 9, 2022 20:56
timothycoleman
timothycoleman reviewed Feb 14, 2022
View reviewed changes
src/EventStore.Client/EventStoreClientBase.cs Outdated
private async Task<HttpRequestMessage> CreateRequest(string path, string query, HttpMethod method, UserCredentials? credentials, CancellationToken cancellationToken) {
credentials ??= Settings.DefaultCredentials;

var scheme = Settings.ConnectivitySettings.Address.Scheme;
Copy link
Contributor

@timothycoleman timothycoleman Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

not certain, but is this necessarily the right scheme. i wonder if ConnectivitySettings.Address is necessarily populated for clusters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use: ConnectivitySettings.Insecure

Copy link
Contributor

Choose a reason for hiding this comment

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

is ConnectivitySettings.Insecure definitely set for single node? im not certain either way

Copy link
Contributor

Choose a reason for hiding this comment

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

(this is now in httpfallback.cs)

timothycoleman
timothycoleman requested changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

looks good, comments and suggestions above ^

@pvanbuijtene pvanbuijtene force-pushed the viable-perst branch 2 times, most recently from 1afa205 to 102c7f3 Compare December 16, 2022 09:52
hayley-jean
hayley-jean previously approved these changes Dec 16, 2022
View reviewed changes
@timothycoleman
Copy link
Contributor

looks good, i think there's just the two unresolved conversations above left

@pvanbuijtene pvanbuijtene force-pushed the viable-perst branch 2 times, most recently from a93832a to 797296c Compare December 20, 2022 10:26
@timothycoleman
Copy link
Contributor

looks good to me. if @thefringeninja is happy with the last commit i think we can squash and merge

thefringeninja
thefringeninja suggested changes Dec 22, 2022
View reviewed changes
src/EventStore.Client/EventStoreClientConnectivitySettings.cs Outdated
set {
_insecure = value;

_defaultAddress = new UriBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing Insecure should not result in changing the set address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only setting the default address which should be returned incase there is no address set. It won't change the address once it's set. This is done to keep the Address.Scheme in sync with the Insecure setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored it a bit to resemble this behavior.

thefringeninja
thefringeninja previously approved these changes Dec 23, 2022
View reviewed changes
timothycoleman
timothycoleman approved these changes Dec 23, 2022
View reviewed changes
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

👍

@thefringeninja thefringeninja merged commit e4dc5b3 into EventStore:master Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jageall jageall jageall left review comments

@oskardudycz oskardudycz oskardudycz requested changes

@thefringeninja thefringeninja thefringeninja approved these changes

@timothycoleman timothycoleman timothycoleman approved these changes

@hayley-jean hayley-jean hayley-jean left review comments

Assignees

@pvanbuijtene pvanbuijtene

Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@pvanbuijtene @ylorph @azelinskyy-mechanicadvisor @timothycoleman @thefringeninja @jageall @hayley-jean @oskardudycz