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

initial bulk publish impl for java #789

Merged
merged 44 commits into from
Jan 19, 2023

Conversation

mukundansundar
Copy link
Contributor

@mukundansundar mukundansundar commented Sep 26, 2022

Signed-off-by: Mukundan Sundararajan 65565396+mukundansundar@users.noreply.github.com

Description

initial bulk publish implementation for Java SDK

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Since bulk subscribe is already implemented, this PR closes #778

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
String contentType = entry.getContentType();

// Serialize event into bytes
if (!Strings.isNullOrEmpty(contentType) && objectSerializer instanceof DefaultObjectSerializer) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a problem we need to fix in other places in the SDK. Using the default serializer should not trigger different behavior. It should be based on the content-type only.

Copy link
Contributor Author

@mukundansundar mukundansundar Sep 29, 2022

Choose a reason for hiding this comment

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

this is a change that needs to be done SDK wide and not only for one api ... If it is is only changed here that might cause serialized events from this API to become incompatible with the other normal publish API ...

Copy link
Contributor

@johnewart johnewart left a comment

Choose a reason for hiding this comment

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

Just a few questions / observations, mostly for my own edification - you don't need to block on them

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar mukundansundar marked this pull request as ready for review September 28, 2022 20:42
@mukundansundar mukundansundar requested review from a team as code owners September 28, 2022 20:42
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar
Copy link
Contributor Author

The build failures here are wrt Config API changes which is handled in the PR #740.

Locally also verified that the PubsubIT passes as expected ....
cc @dapr/maintainers-java-sdk

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar
Copy link
Contributor Author

As discussed offline, this PR only contains the gRPC implementation for Bulk Publish

*/
@Override
public <T> Mono<BulkPublishResponse<T>> publishEvents(BulkPublishRequest<T> request) {
return DaprException.wrapMono(new UnsupportedOperationException());
Copy link
Member

Choose a reason for hiding this comment

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

I assume this will be implemented next, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@artursouza as we discussed previously, we were saying that HTTP impl is not needed, and that HTTP Client is going to be deprecated. #790

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar
Copy link
Contributor Author

mukundansundar commented Jan 11, 2023

@dapr/maintainers-java-sdk see the latest checks for this PR itself. It required only 2 runs.
The older one https://github.com/dapr/java-sdk/actions/runs/3871888965 required 4 attempts. (Before adding doc content)

For some reason the Kafka subscription is flaky.

Additionally I am seeing flakiness in other validations as well. Please see the logs

@mukundansundar
Copy link
Contributor Author

@artursouza any updates on this PR?

artursouza and others added 4 commits January 17, 2023 18:32
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@mukundansundar
Copy link
Contributor Author

@dapr/maintainers-java-sdk see the latest checks for this PR itself. It required only 2 runs. The older one https://github.com/dapr/java-sdk/actions/runs/3871888965 required 4 attempts. (Before adding doc content)

For some reason the Kafka subscription is flaky.

Additionally I am seeing flakiness in other validations as well. Please see the logs

Changed the examples to use Redis, that removes the flakiness.

Signed-off-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Merging #789 (99639df) into master (b83661d) will increase coverage by 0.35%.
The diff coverage is 67.56%.

@@             Coverage Diff              @@
##             master     #789      +/-   ##
============================================
+ Coverage     77.62%   77.97%   +0.35%     
- Complexity     1161     1258      +97     
============================================
  Files           105      116      +11     
  Lines          3647     3887     +240     
  Branches        419      457      +38     
============================================
+ Hits           2831     3031     +200     
- Misses          603      625      +22     
- Partials        213      231      +18     
Impacted Files Coverage Δ
.../client/domain/BulkPublishResponseFailedEntry.java 0.00% <0.00%> (ø)
...o/dapr/client/domain/BulkSubscribeAppResponse.java 0.00% <0.00%> (ø)
...r/client/domain/BulkSubscribeAppResponseEntry.java 0.00% <0.00%> (ø)
.../client/domain/BulkSubscribeAppResponseStatus.java 0.00% <0.00%> (ø)
...va/io/dapr/client/domain/BulkSubscribeMessage.java 0.00% <0.00%> (ø)
.../dapr/client/domain/BulkSubscribeMessageEntry.java 0.00% <0.00%> (ø)
sdk/src/main/java/io/dapr/utils/TypeRef.java 78.33% <44.44%> (-5.99%) ⬇️
...ava/io/dapr/client/domain/BulkPublishResponse.java 50.00% <50.00%> (ø)
...c/main/java/io/dapr/client/AbstractDaprClient.java 84.76% <55.55%> (-2.74%) ⬇️
...n/java/io/dapr/client/domain/BulkPublishEntry.java 58.82% <58.82%> (ø)
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mukundansundar
Copy link
Contributor Author

mukundansundar commented Jan 19, 2023

@dapr/maintainers-java-sdk Had a discussion with @shubham1172 on whether to merge BulkSubscribeMessageEntry and BulkPublishEntry classes into a single class or not. As of now what we feel is that it is clearer to have two different set of classes for BulkPublish and BulkSubscribe and not have a common class in-between. There is some duplication, but since the APIs are orthogonal they can be used independently of each other and it will be clearer if we have two sets of classes and not have a common BulkMessageEntry class which will be used by BulkPublish and BulkSubscribe APIs.

@artursouza artursouza merged commit 81591b9 into dapr:master Jan 19, 2023
@mukundansundar mukundansundar deleted the bulk-publish-sdk branch January 19, 2023 21:01
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.

Bulk PubSub implementation
4 participants