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

Feat(client/ C#): Update TopologyRequestCommand and UpdateRetriesCommand for retries #329

Merged
merged 4 commits into from
Oct 6, 2021
Merged

Feat(client/ C#): Update TopologyRequestCommand and UpdateRetriesCommand for retries #329

merged 4 commits into from
Oct 6, 2021

Conversation

geekusa33
Copy link
Contributor

closes #184 , #183

Updated the interfaces, methods, and main application to use asyncRetryStrategy.

Please let me know if I have missed anything

@geekusa33 geekusa33 requested a review from ChrisKujawa as a code owner October 4, 2021 15:19
@CLAassistant
Copy link

CLAassistant commented Oct 4, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@ChrisKujawa ChrisKujawa left a comment

Choose a reason for hiding this comment

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

Hey @askalik thanks so much for your contribution :)

Could you please create two separate PR's next time, to separate the concerns :)

You currently have two commits, please squash them (the first one has no real change).

Sorry, I had to forget to mention, we also need to test this. For that please add also a TestCaseData object on this provider https://github.com/camunda-community-hub/zeebe-client-csharp/blob/master/Client.UnitTests/TestDataProvider.cs, such that the parameterized test is running also for this command. Feel free to ask any questions you might have.

Client/Impl/Commands/UpdateRetriesCommand.cs Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
update to re-add send method that was removed erroneously.

Feature: add retries logic

Adding retry logic to TopologyRequestCommand and UpdateRetriesCommand

feat(client/ C#): allow with retry

updates the UpdateRetriesCommandStep1 and TopologyRequestCommand to use FinalCommandWithRetrySteps.  Issue #183  #184
@geekusa33
Copy link
Contributor Author

@Zelldon I squashed the commits, and re-added the send method.

Working on getting the test updated. Is it the topology that is to be added, updated with retries - or both?

@ChrisKujawa
Copy link
Collaborator

Thanks @askalik

yes please add two new test case data objects here https://github.com/camunda-community-hub/zeebe-client-csharp/blob/master/Client.UnitTests/TestDataProvider.cs#L13

Example for the topology:

yield return new TestCaseData(
                    new TopologyRequest
                    {
                        ProcessInstanceKey = 12113
                    }, new TopologyResponse(),
                    (RequestCreator<ITopology>)
                    (zeebeClient => zeebeClient.TopologyRequest()));

…Retries

feat: add retries

update to re-add send method that was removed erroneously.

Feature: add retries logic

Adding retry logic to TopologyRequestCommand and UpdateRetriesCommand

feat(client/ C#): allow with retry

updates the UpdateRetriesCommandStep1 and TopologyRequestCommand to use FinalCommandWithRetrySteps.  Issue #183  #184
…h-retries' of https://github.com/askalik/zeebe-client-csharp into feature-TopologyRequestCommand-UpdateRetriesCommand-with-retries
Copy link
Collaborator

@ChrisKujawa ChrisKujawa left a comment

Choose a reason for hiding this comment

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

🎖️

yield return new TestCaseData(
new TopologyRequest
{
ProcessInstanceKey = 12113
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry my example was bit wrong 🙈 here the key makes no sense, I'm wondering that this compiles 😆

But it is fine I can fix it later :) I will merge it anyway Thanks @askalik for all your efforts 🚀

@ChrisKujawa ChrisKujawa merged commit 4d0a8db into camunda-community-hub:master Oct 6, 2021
ChrisKujawa added a commit that referenced this pull request Oct 6, 2021
Previous PR was merged without checking CI action, it misses several imports, parameters or wrong types have been used. We should take a closer look next time.

related PR #329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I can use the client to send UpdateRetriesCommand with retries
3 participants