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

Refactor Publishing #34

Merged
merged 10 commits into from
Oct 27, 2021
Merged

Refactor Publishing #34

merged 10 commits into from
Oct 27, 2021

Conversation

rnarubin
Copy link
Collaborator

@rnarubin rnarubin commented Oct 14, 2021

This changeset completely refactors publishing to be based on async
Sinks, and to port the googlepubsub publisher to use gRPC instead of the
JSON REST api. Together, these should make the library easier to use,
compose better with the broader rust async ecosystem, and provide better
performance for publishing.

This additionally simplifies the crate hierarchy and some of the crate
features, as the divisions only made much sense when publishing and
consuming were separate implementations. Now backends are expected to
provide publishing and consuming together at the same time.

Most of this code is adapted from the private crate standard-hedwig,
which has been a proving ground for the publishing API before it could
be incorporated into this open-source crate, and has seen effective
operation in production.

There are still some changes to follow in later commits to finalize the
API before publishing as a major release, as noted in some of the source
comments. The general shape of the library is largely ready, however,
and is worth beginning a review.
Changes added, this is all set to go.

@rnarubin rnarubin requested a review from nagisa October 14, 2021 00:29
@rnarubin rnarubin self-assigned this Oct 14, 2021
@rnarubin
Copy link
Collaborator Author

cc @kyle-dorman I can't add you as a reviewer, not sure why

Copy link
Collaborator

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Seems very nice so far!

src/backends/googlepubsub/mod.rs Outdated Show resolved Hide resolved
src/backends/googlepubsub/publisher.rs Outdated Show resolved Hide resolved
src/backends/googlepubsub/publisher.rs Show resolved Hide resolved
Lints were previously set to run for each platform, which isn't
particularly useful. They're now on ubuntu only

Additionally the lints were run without features enabled, which would
miss most of the code. Now they run with all features. We could consider
running a lint matrix against some subset of features too, but I'm not
sure if that would be useful.

This exposed a few places in the code which failed the
now-feature-checking lints, these have been fixed
This changeset completely refactors publishing to be based on async
Sinks, and to port the googlepubsub publisher to use gRPC instead of the
JSON REST api. Together, these should make the library easier to use,
compose better with the broader rust async ecosystem, and provide better
performance for publishing.

This additionally simplifies the crate hierarchy and some of the crate
features, as the divisions only made much sense when publishing and
consuming were separate implementations. Now backends are expected to
provide publishing and consuming together at the same time.

Most of this code is adapted from the private crate standard-hedwig,
which has been a proving ground for the publishing API before it could
be incorporated into this open-source crate, and has seen effective
operation in production.

There are still some changes to follow in later commits to finalize the
API before publishing as a major release, as noted in some of the source
comments. The general shape of the library is largely ready, however,
and is worth beginning a review.
This resolves some lint name problems and provides BTreeMap::retain
This change adds mechanisms to 1) inform users when a (possibly
buffered) message has been published, and 2) retry operations, both now
in the user's message type. Previously retries were only possible in an
opaque pubsub grpc type, and the publish listening wasn't available.
}

#[derive(Debug)]
struct TransformedMessage {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like something that might make sense to have provided by the library in a generic way, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's the AcknowledgeableMessage type that the user could employ (and a pubsub-specific alias PubSubMessage). Now that i think about it, I should alias the TransformedMessage type to one of those.

I've also thought about adding a map function to AcknowledgeableMessage so that a user could operate on the message and leave the token as-is, but then this goes down the rabbit hole of monad-ish types (implement flatmap? flatten? fallible mapping? async mapping?). Instead i left the fields as public so the user can construct them as they wish

examples/googlepubsub.rs Show resolved Hide resolved
src/backends/googlepubsub/publisher.rs Show resolved Hide resolved
src/backends/googlepubsub/publisher.rs Show resolved Hide resolved
src/backends/googlepubsub/publisher.rs Outdated Show resolved Hide resolved
- print error descriptions in PublishError::Display
- use PubSubMessage type for carrying ack tokens in pubsub example
@rnarubin rnarubin merged commit ff16892 into standard-ai:v5.x Oct 27, 2021
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.

2 participants