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

Emit larger chunks in HTTP and gRPC consumers #330

Merged
merged 11 commits into from
Jan 5, 2022
Merged

Conversation

bastewart
Copy link
Contributor

CE3 port/tweak of #329.

There are two subtle user-affecting changes as a result of this:

  1. In both consumer variants if any message in a chunk fails to
    decode the errorHandler is invoked before any successfully
    decoded messages in the chunk are emitted downstream; even
    if the message which failed to decode comes after the successful
    ones.
  2. In the gRPC variant alone the stream may fail sooner if an internal
    failure occurs in the Java library. This should not matter at all.

bastewart and others added 5 commits November 23, 2021 17:31
This should improve performance as it avoids calling looping through
`takeNextElements` as long as elements are still available.
Avoids wrapping potential `null` into an `Option`. Also reduces number
of `delay` blocks as there were no external side effects.
This applies to both the gRPC and HTTP consumers. It could lead to
performance improvements in applications which are sensitive to the
chunksize.

There is a subtle user-facing change as a result: if any message in a
chunk fails to decode the `errorHandler` is invoked _before_ any
successfully decoded messages in the chunk are emitted downstream; even
if the message which failed to decode comes after the successful ones.

This change is probably not a problem though...
@bastewart bastewart requested a review from CremboC November 23, 2021 19:00
@bastewart
Copy link
Contributor Author

@istreeter, @CremboC has got the PubSub emulator working with gRPC so we may be able to commit benchmarks into the repo...

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #330 (6de7203) into master (74ccdc6) will increase coverage by 12.49%.
The diff coverage is 85.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #330       +/-   ##
===========================================
+ Coverage   35.00%   47.49%   +12.49%     
===========================================
  Files          29       29               
  Lines         300      299        -1     
  Branches        8        6        -2     
===========================================
+ Hits          105      142       +37     
+ Misses        195      157       -38     
Impacted Files Coverage Δ
...ubsub/producer/grpc/internal/PubsubPublisher.scala 100.00% <ø> (+100.00%) ⬆️
...ve/pubsub/consumer/grpc/PubsubGoogleConsumer.scala 50.00% <75.00%> (+50.00%) ⬆️
...tive/pubsub/consumer/http/PubsubHttpConsumer.scala 66.66% <75.00%> (+26.66%) ⬆️
...bsub/consumer/grpc/internal/PubsubSubscriber.scala 88.23% <91.66%> (+88.23%) ⬆️
...com/permutive/pubsub/consumer/ConsumerRecord.scala 66.66% <0.00%> (+66.66%) ⬆️
...bsub/producer/grpc/internal/DefaultPublisher.scala 75.00% <0.00%> (+75.00%) ⬆️
.../pubsub/producer/grpc/internal/FutureInterop.scala 84.61% <0.00%> (+84.61%) ⬆️
...ve/pubsub/producer/grpc/GooglePubsubProducer.scala 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74ccdc6...6de7203. Read the comment docs.

@istreeter istreeter mentioned this pull request Nov 25, 2021
@istreeter
Copy link
Contributor

Hi @bastewart - I was just wondering when this change might get merged and released? No pressure, and not urgent - it's just so I know when to expect it. Many thanks!

@CremboC CremboC self-requested a review January 4, 2022 17:38
@CremboC
Copy link
Contributor

CremboC commented Jan 4, 2022

Hey @istreeter , I'm trying to get the GRPC tests to run. Hopefully we can get them to work this week and then merge and release.

@CremboC CremboC merged commit 27466a7 into master Jan 5, 2022
@CremboC CremboC deleted the increase-chunk-size branch January 5, 2022 16:19
@CremboC
Copy link
Contributor

CremboC commented Jan 5, 2022

@istreeter do you need a CE2 version of this PR as well?

@istreeter
Copy link
Contributor

Hey @CremboC a CE2 version would be extremely helpful. I hope I'm not asking for too much 😇. If it is beyond scope for you to release a CE2 version then I will release a version from my forked repo.

CremboC added a commit that referenced this pull request Jan 6, 2022
@CremboC CremboC mentioned this pull request Jan 6, 2022
@benjben benjben mentioned this pull request Jan 7, 2022
15 tasks
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.

4 participants