Support using process instance migration from the zeebe client #1012

korthout · 2024-01-02T14:01:29Z

Description

This adds support for the new MigrateProcessInstance RPC to ZPT, such that users can try migrating process instances from ZPT. This allows testing migrations before doing so in production.

Note

This does not add assertions for the migration. That is out of scope.

Additionally, this fixes a test case that no longer added value. The test case was intended to ensure that we don't forget supporting RPCs in ZPT, but the implementation of the test did not function correctly anymore. Now it is able to correctly detect unsupported RPCs again, which highlighted that streamActivatedJobs is not supported by ZPT.

Related issues

closes #972

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually

Documentation:

Javadoc has been written
The documentation is updated

Simplifies the map and filter by placing map before filter

Currently, only the bindService method needs to be ignored as it is explicitly declared by GatewayImplBase. But the other methods from the interfaces BindableService and AsyncService are not retrieved by getDeclaredMethods. For this, we'll need to use getMethods. However, that method will also pass undeclared methods like equals which we'll need to explicitly ignore. This we can easily achieve using a list of strings.

This test was not testing what was intended. It wants to ensure that all the RPCs of the gateway are supported by the GrpcToLogStreamGateway. It does this by verifying that the GrpcToLogStreamGateway declares an implementation for each of the gateway's methods. However, the getDeclaredMethods doesn't return those. Possibly these were moved to AsyncService at some point. Note that this is generated code and that this may have been changed by changes upstream in the grpc library. Instead, we must use getMethods to retrieve all methods available on this class (also those defined through default implementations on its interfaces). When we do that, we must also ignore a bunch of undeclared methods, like equals and hashCode. What we're left with is a failing test that fails for two missing methods: - streamActivatedJobs - migrateProcessInstance

Fixes failing test case: - GrpcToLogStreamGatewayTest.testImplementsGatewayEndpoint#migrateProcessInstance

It's not yet possible to support this method as it functions differently from the other RPCS. Typically, the RPCs are implemented by appending a command to the log stream, but for streaming RPCs it works differently. Instead, the gateway opens a stream through which the broker can stream events. As I understand the implementation in Zeebe, this works without appending a command. In order to support this in ZPT, some new mechanism must be added that allows streaming events to the calling client.

Adds a simple example test case that shows we can migrate a process instance through the zeebe client in ZPT.

github-actions · 2024-01-02T14:12:30Z

Test Results

47 files ± 0 47 suites ±0 1m 40s ⏱️ +3s
126 tests +20 126 ✅ +20 0 💤 ±0 0 ❌ ±0
402 runs +60 402 ✅ +60 0 💤 ±0 0 ❌ ±0

Results for commit 46a2e5c. ± Comparison against base commit fe33845.

♻️ This comment has been updated with latest results.

berkaycanbc

You left the code better than you found, thanks @korthout! 🚀

I only requested one possible change.

berkaycanbc · 2024-01-02T15:15:23Z

engine/src/main/java/io/camunda/zeebe/process/test/engine/GrpcResponseMapper.java

+        .forEach(migrationPlanBuilder::addMappingInstructions);
+    final var migrationPlan = migrationPlanBuilder.build();
+
+    return MigrateProcessInstanceRequest.newBuilder()


❌❓In the original response, we do not return any data. Is this intended to return data here for some purpose? @korthout

Good point. I tried to be too fast, and missed that I used *Request rather than *Response. The request was asking for this data, so I added it without looking any further.

I've fixed it up to:

private GeneratedMessageV3 createMigrateProcessInstanceResponse() { return MigrateProcessInstanceResponse.getDefaultInstance(); }

This test is quite important, because it ensure we don't forget supporting RPCs in ZPT. To help clarify this, I've added instructions in case the test fails.

berkaycanbc

LGTM! 🚀

korthout · 2024-01-02T15:33:27Z

bors merge

1012: Support using process instance migration from the zeebe client r=korthout a=korthout ## Description  This adds support for the new `MigrateProcessInstance` RPC to ZPT, such that users can try migrating process instances from ZPT. This allows testing migrations before doing so in production. > [!NOTE] > This does not add assertions for the migration. That is out of scope. Additionally, this fixes a test case that no longer added value. The test case was intended to ensure that we don't forget supporting RPCs in ZPT, but the implementation of the test did not function correctly anymore. Now it is able to correctly detect unsupported RPCs again, which [highlighted that `streamActivatedJobs` is not supported by ZPT](camunda/camunda#11231 (comment)). ## Related issues  closes #972  ## Definition of Done  _Not all items need to be done depending on the issue and the pull request._ Code changes: * [x] The changes are backwards compatibility with previous versions * [ ] If it fixes a bug then PRs are created to backport the fix Testing: * [x] There are unit/integration tests that verify all acceptance criterias of the issue * [x] New tests are written to ensure backwards compatibility with further versions * [ ] The behavior is tested manually Documentation: * [ ] Javadoc has been written * [ ] The documentation is updated Co-authored-by: Nico Korthout <[email protected]>

ghost · 2024-01-02T15:45:17Z

Build failed:

Test summary

korthout · 2024-01-02T16:17:29Z

Known flaky test

bors retry

1012: Support using process instance migration from the zeebe client r=korthout a=korthout ## Description  This adds support for the new `MigrateProcessInstance` RPC to ZPT, such that users can try migrating process instances from ZPT. This allows testing migrations before doing so in production. > [!NOTE] > This does not add assertions for the migration. That is out of scope. Additionally, this fixes a test case that no longer added value. The test case was intended to ensure that we don't forget supporting RPCs in ZPT, but the implementation of the test did not function correctly anymore. Now it is able to correctly detect unsupported RPCs again, which [highlighted that `streamActivatedJobs` is not supported by ZPT](camunda/camunda#11231 (comment)). ## Related issues  closes #972  ## Definition of Done  _Not all items need to be done depending on the issue and the pull request._ Code changes: * [x] The changes are backwards compatibility with previous versions * [ ] If it fixes a bug then PRs are created to backport the fix Testing: * [x] There are unit/integration tests that verify all acceptance criterias of the issue * [x] New tests are written to ensure backwards compatibility with further versions * [ ] The behavior is tested manually Documentation: * [ ] Javadoc has been written * [ ] The documentation is updated Co-authored-by: Nico Korthout <[email protected]>

ghost · 2024-01-02T16:23:08Z

Build failed:

Test summary

korthout · 2024-01-02T16:23:49Z

Same flaky test

bors retry

ghost · 2024-01-02T16:32:44Z

Build succeeded!

And happy new year! 🎉

Test summary

korthout · 2024-01-03T09:12:16Z

/backport

I forgot to backport it to the release branch, which had already been cut.

backport-action · 2024-01-03T09:12:31Z

Successfully created backport PR for release-8.4.0:

[Backport release-8.4.0] Support using process instance migration from the zeebe client #1013

1013: [Backport release-8.4.0] Support using process instance migration from the zeebe client r=github-actions[bot] a=backport-action # Description Backport of #1012 to `release-8.4.0`. relates to #972 Co-authored-by: Nico Korthout <[email protected]>

korthout added 6 commits January 2, 2024 13:51

refactor: remove duplicate getName

6e20e51

Simplifies the map and filter by placing map before filter

fix: support migrate process instance rpc

cb3a81a

Fixes failing test case: - GrpcToLogStreamGatewayTest.testImplementsGatewayEndpoint#migrateProcessInstance

test: support migrating process instances

c6e082b

Adds a simple example test case that shows we can migrate a process instance through the zeebe client in ZPT.

korthout marked this pull request as ready for review January 2, 2024 14:12

korthout requested a review from berkaycanbc January 2, 2024 14:12

berkaycanbc requested changes Jan 2, 2024

View reviewed changes

korthout force-pushed the korthout-972-instance-migration-support branch from 9ec9114 to aac9033 Compare January 2, 2024 15:20

korthout requested a review from berkaycanbc January 2, 2024 15:22

korthout added 2 commits January 2, 2024 16:22

fix: support instance migration response

0c24ced

docs: describe what to do when test fails

46a2e5c

This test is quite important, because it ensure we don't forget supporting RPCs in ZPT. To help clarify this, I've added instructions in case the test fails.

korthout force-pushed the korthout-972-instance-migration-support branch from aac9033 to 46a2e5c Compare January 2, 2024 15:22

berkaycanbc approved these changes Jan 2, 2024

View reviewed changes

ghost merged commit 67c23d8 into main Jan 2, 2024
11 checks passed

ghost deleted the korthout-972-instance-migration-support branch January 2, 2024 16:32

korthout added the backport release-8.4.0 label Jan 3, 2024

backport-action mentioned this pull request Jan 3, 2024

[Backport release-8.4.0] Support using process instance migration from the zeebe client #1013

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support using process instance migration from the zeebe client #1012

Support using process instance migration from the zeebe client #1012

korthout commented Jan 2, 2024

github-actions bot commented Jan 2, 2024 •

edited

Loading

berkaycanbc left a comment

berkaycanbc Jan 2, 2024

korthout Jan 2, 2024 •

edited

Loading

berkaycanbc left a comment

korthout commented Jan 2, 2024

ghost commented Jan 2, 2024

korthout commented Jan 2, 2024

ghost commented Jan 2, 2024

korthout commented Jan 2, 2024

ghost commented Jan 2, 2024

korthout commented Jan 3, 2024

backport-action commented Jan 3, 2024

Support using process instance migration from the zeebe client #1012

Support using process instance migration from the zeebe client #1012

Conversation

korthout commented Jan 2, 2024

Description

Related issues

Definition of Done

github-actions bot commented Jan 2, 2024 • edited Loading

Test Results

berkaycanbc left a comment

Choose a reason for hiding this comment

berkaycanbc Jan 2, 2024

Choose a reason for hiding this comment

korthout Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

berkaycanbc left a comment

Choose a reason for hiding this comment

korthout commented Jan 2, 2024

ghost commented Jan 2, 2024

korthout commented Jan 2, 2024

ghost commented Jan 2, 2024

korthout commented Jan 2, 2024

ghost commented Jan 2, 2024

korthout commented Jan 3, 2024

backport-action commented Jan 3, 2024

github-actions bot commented Jan 2, 2024 •

edited

Loading

korthout Jan 2, 2024 •

edited

Loading