Merged
Conversation
benbjohnson
requested changes
Sep 12, 2023
Contributor
Author
|
I added a |
benbjohnson
requested changes
Sep 14, 2023
client.go
Outdated
Comment on lines
8
to
18
| // SubscribeEvents subscribes to events from the default (localhost:20202) | ||
| // LiteFS node. | ||
| func SubscribeEvents() EventSubscription { | ||
| return DefaultClient.SubscribeEvents() | ||
| } | ||
|
|
||
| // MonitorPrimary monitors the primary status of the LiteFS cluster via the | ||
| // default (localhost:20202) LiteFS node's event stream. | ||
| func MonitorPrimary() PrimaryMonitor { | ||
| return DefaultClient.MonitorPrimary() | ||
| } |
Collaborator
There was a problem hiding this comment.
I don't think these add much value since they're one-liners. I think you can just remove them.
Comment on lines
+29
to
+34
| // DefaultClient is a client for communicating with the default | ||
| // (localhost:20202) LiteFS node. | ||
| var DefaultClient = &Client{ | ||
| URL: "http://localhost:20202", | ||
| HTTP: http.DefaultClient, | ||
| } |
Collaborator
There was a problem hiding this comment.
This works. You could also have NewClient() return a client with defaults set.
Comment on lines
+17
to
+27
| // EventSubscription monitors a LiteFS node for published events. | ||
| type EventSubscription interface { | ||
| // Next attempts to read the next event from the LiteFS node. An error is | ||
| // returned if the request fails. Calling `Next()` again after an error will | ||
| // initiate a new HTTP request. ErrClosed is returned if the EventSubscription | ||
| // is closed while this method is blocking. | ||
| Next() (*Event, error) | ||
|
|
||
| // Close aborts any in-progress requests to the LiteFS node. | ||
| Close() | ||
| } |
Collaborator
There was a problem hiding this comment.
I think it was better just having the regular struct. I'm not a huge fan of interfaces unless you really need them. I don't see users mocking this out and if they do, it's pretty easy to mock out the actual HTTP endpoint.
Collaborator
|
@btoews Sorry for the slow responses. I've been slammed lately. Just some minor feedback and then we can merge it in. 👍 |
benbjohnson
approved these changes
Sep 14, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds client APIs for working with the event streams added in superfly/litefs#401. A lower level
EventSubscriptiontype is added for subscribing to raw events. More generally useful is thePrimaryMonitortype that continuously monitors the event stream for changes in the cluster primary, allowing current information about the primary to be fetched immediately.Here's a preview of the godocs.