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

Support consuming N message in a single command #198

Merged
merged 2 commits into from Jul 30, 2023
Merged

Conversation

zklgame
Copy link
Contributor

@zklgame zklgame commented Jul 28, 2023

For #190.
Support both internal channel command and signal command.

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #198 (e8680ab) into main (757eed4) will increase coverage by 0.12%.
The diff coverage is 77.35%.

@@             Coverage Diff              @@
##               main     #198      +/-   ##
============================================
+ Coverage     71.36%   71.49%   +0.12%     
- Complexity      366      376      +10     
============================================
  Files            59       60       +1     
  Lines          1526     1575      +49     
  Branches        135      139       +4     
============================================
+ Hits           1089     1126      +37     
- Misses          368      379      +11     
- Partials         69       70       +1     
Files Changed Coverage Δ
...low/core/communication/InternalChannelCommand.java 73.68% <66.66%> (-26.32%) ⬇️
...io/iworkflow/core/communication/SignalCommand.java 73.68% <66.66%> (-26.32%) ⬇️
...n/java/io/iworkflow/core/command/SuperCommand.java 75.00% <75.00%> (ø)
...java/io/iworkflow/core/command/CommandRequest.java 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Value.Immutable
public abstract class InternalChannelCommand implements BaseCommand {

public abstract String getChannelName();
public abstract int getCount();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it will be much easier to understand/maintain if we create a new structure for this SuperCommand implements BaseCommand.

It's a bit confusing to say, a signal command contain multiple signal commands.
We can just put the helper in this file:

public static SuperCommand create(final String commandId, final String channelName, final int count) {}
public static SuperCommand create(final String channelName, final int count) {}

And the above code in ComandRequest will look cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is better!

/**
* Create one or many internal channel commands.
*
* @param commandId required.
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably not aware of -- there are two usage of commandId:

  • for retrieving commandResults, user can use commandId
  • more importantly, forAnyCommandCombinationCompleted use commandId to identify different combination.
    Here means all the commands created will share the same commandId. So we probably should make this comment clear here that this is the expectation. Otherwise they use go back to create multiple InternalCommand themselves.

@zklgame zklgame merged commit 3409ea7 into main Jul 30, 2023
3 checks passed
@zklgame zklgame deleted the zklgame/issue_190 branch July 31, 2023 06:05
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.

None yet

2 participants