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

Update Zeebe to 8.2.0-rc1 #275

Merged
merged 9 commits into from
Mar 29, 2023

Conversation

skayliu
Copy link
Contributor

@skayliu skayliu commented Mar 28, 2023

Description

skayliu added 7 commits March 25, 2023 21:30
The Engine method was changed in Zeebe. The EngineConfiguration parameter was added, this also needs to be removed here.

The createEngineProcessors method was changed in Zeebe.One of the parameters was removed, this also needs to be removed here.

The createEngineProcessors method was changed in Zeebe.The JobStreamer parameter was added, this also needs to be added here.
@skayliu skayliu changed the title Update Zeebe to 8.2.0-alpha5 Update Zeebe to 8.2.0-rc1 Mar 28, 2023
@skayliu
Copy link
Contributor Author

skayliu commented Mar 28, 2023

Hey, @saig0 @Zelldon, The should broadcast signal test case works well on my pc, could you check it? Thank you.

@skayliu
Copy link
Contributor Author

skayliu commented Mar 28, 2023

And should increase the time test may fail, the related Zeebe PR Message TTL Checker goes idle when needed

@saig0
Copy link
Collaborator

saig0 commented Mar 29, 2023

@skayliu thank you for your contribution. 🎉

I will have a look at your changes soon. 👀

@saig0 saig0 removed the request for review from ChrisKujawa March 29, 2023 11:12
Copy link
Collaborator

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@skayliu nice job. 🎉 The changes look good. 🚀

I had a look at the failing test cases and found a solution. Please check my comments. 🍪

Comment on lines 1119 to 1179
// given
zeebeClient = ZeebeClient.newClientBuilder().usePlaintext().build()

zeebeClient.newDeployResourceCommand()
.addProcessModel(
Bpmn.createExecutableProcess("process")
.startEvent()
.signal("top-level-start-signal")
.endEvent()
.done(), "process.bpmn"
)
.send()
.join()

// when
zeebeClient
.newBroadcastSignalCommand()
.signalName("top-level-start-signal")
.variables(mapOf("signal" to "broadCasted"))
.send()
.join()

// then
val signalSubscriptionRecord = zeebeEngine
.signalSubscriptionRecords()
.withIntent(SignalSubscriptionIntent.CREATED)
.firstOrNull()

assertThat(signalSubscriptionRecord)
.describedAs("When deploy a process with top-level signal start event a signal subscription record is created")
.isNotNull
if (signalSubscriptionRecord != null) {
assertThat(signalSubscriptionRecord.value.signalName).isEqualTo("top-level-start-signal")
}

val signalRecord = zeebeEngine
.signalRecords()
.withIntent(SignalIntent.BROADCASTED)
.firstOrNull()

assertThat(signalRecord)
.describedAs("The Signal is broadcasted well")
.isNotNull

if (signalRecord != null) {
assertThat(signalRecord.value.signalName).isEqualTo("top-level-start-signal")
assertThat(signalRecord.value.variables)
.describedAs("The signal broadcast command can has variables")
.containsEntry("signal", "broadCasted")
}

val processInstance = zeebeEngine
.processInstanceRecords()
.withIntent(ProcessInstanceIntent.ELEMENT_COMPLETED)
.withElementType(BpmnElementType.PROCESS)
.firstOrNull()

assertThat(processInstance)
.describedAs("The top-level signal start event can be triggered by signal broadcast command")
.isNotNull

Copy link
Collaborator

Choose a reason for hiding this comment

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

❌ This test case can fail because of the async nature of the exporter.

If we verify records on the stream, we should wrap the verification inside a await.untilAsserted { ... } block. This is part of Awaitility to deal with async code.

If I apply the modification, the test case works fine.

The verifications of the signal records could be removed because this is part of Zeebe's workflow engine itself. To verify the signal broadcast, it is enough to verify that the process instance is created/completed.

        // given
        zeebeClient = ZeebeClient.newClientBuilder().usePlaintext().build()

        zeebeClient.newDeployResourceCommand()
            .addProcessModel(
                Bpmn.createExecutableProcess("signal-process")
                    .startEvent()
                    .signal("top-level-start-signal")
                    .endEvent()
                    .done(), "process.bpmn"
            )
            .send()
            .join()

        // when
        zeebeClient
            .newBroadcastSignalCommand()
            .signalName("top-level-start-signal")
            .variables(mapOf("signal" to "broadCasted"))
            .send()
            .join()

        // then
        await.untilAsserted {
            val processInstance = zeebeEngine
                .processInstanceRecords()
                .withBpmnProcessId("signal-process")
                .withIntent(ProcessInstanceIntent.ELEMENT_COMPLETED)
                .withElementType(BpmnElementType.PROCESS)
                .firstOrNull()

            assertThat(processInstance)
                .describedAs("The top-level signal start event can be triggered by signal broadcast command")
                .isNotNull
        }

skayliu added 2 commits March 29, 2023 21:04
With the latest Zeebe version, the startAtKey parameter is invoked with null. So, the method should be able to handle null.
@skayliu
Copy link
Contributor Author

skayliu commented Mar 29, 2023

@saig0, It's all done, please review again, thank you.

@skayliu skayliu requested a review from saig0 March 29, 2023 13:16
Copy link
Collaborator

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@skayliu lightning fast. ⚡ Thank you for your fast reaction. 🍰

The PR is ready to merge. 🚀

@saig0 saig0 merged commit c76a9b4 into camunda-community-hub:main Mar 29, 2023
@skayliu skayliu deleted the skayliu-signal-event branch April 2, 2023 13:27
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.

2 participants