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

[Forward main] Enable message time-to-live checker configuration #11956

Merged
8 commits merged into from
Mar 8, 2023

Conversation

korthout
Copy link
Member

@korthout korthout commented Mar 7, 2023

Description

Forwards #11947 to main.

Conflicts were resolved by hand in multiple commits. Please review the commits with the Conflicts: section in the commit message with additional care.

Related issues

closes #11922

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 to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

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
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Other teams:
If the change impacts another team an issue has been created for this team, explaining what they need to do to support this change.

Please refer to our review guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Test Results

1 009 files  +    1  1 009 suites  +1   1h 54m 6s ⏱️ + 5m 54s
8 199 tests +221  8 189 ✔️ +221  10 💤 ±0  0 ±0 
8 398 runs  +221  8 388 ✔️ +221  10 💤 ±0  0 ±0 

Results for commit c2f562b. ± Comparison against base commit 8bd8399.

This pull request removes 518 and adds 739 tests. Note that renamed tests count towards both.
DmnEvaluationTest If successfully evaluated, the output ‑ Should return a message pack output[6] value={x=1, z=[1, 2, 3], y=true}
io.camunda.zeebe.engine.processing.bpmn.activity.OutputMappingTest ‑ shouldApplyOutputMapping[0: io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@1ee52741]
io.camunda.zeebe.engine.processing.bpmn.activity.OutputMappingTest ‑ shouldApplyOutputMapping[1: io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@2f5a092e]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=BUSINESS_RULE_TASK, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@16b2e65f, variables={}]]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=CALL_ACTIVITY, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@835d9ae, variables={}]]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=END_EVENT, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@3569ed94, variables={}]]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=EVENT_BASED_GATEWAY, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@b13e6fd, variables={correlationKey=value}]]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=EVENT_SUB_PROCESS, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@a755ca2, variables={}]]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=EXCLUSIVE_GATEWAY, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@11f2ea, variables={}]]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=INTERMEDIATE_CATCH_EVENT, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@3d625460, variables={correlationKey=value}]]
…
DmnEvaluationTest If successfully evaluated, the output ‑ Should return a message pack output[6] value={x=1, y=true, z=[1, 2, 3]}
io.camunda.zeebe.broker.system.configuration.EngineCfgTest ‑ shouldCreateEngineConfigurationFromConfig
io.camunda.zeebe.broker.system.configuration.EngineCfgTest ‑ shouldCreateEngineConfigurationFromDefaults
io.camunda.zeebe.broker.system.configuration.FeatureFlagsCfgTest ‑ shouldDisableMessageTTLCheckerAsyncByDefault
io.camunda.zeebe.broker.system.configuration.FeatureFlagsCfgTest ‑ shouldSetEnableMessageTtlCheckerAsyncFromConfig
io.camunda.zeebe.broker.system.configuration.FeatureFlagsCfgTest ‑ shouldSetEnableMessageTtlCheckerAsyncFromEnv
io.camunda.zeebe.engine.processing.bpmn.activity.OutputMappingTest ‑ shouldApplyOutputMapping[0: io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@525b416f]
io.camunda.zeebe.engine.processing.bpmn.activity.OutputMappingTest ‑ shouldApplyOutputMapping[1: io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@ec50502]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=BUSINESS_RULE_TASK, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@1b68634d, variables={}]]
io.camunda.zeebe.engine.processing.processinstance.CreateProcessInstanceSupportedElementTest ‑ testProcessInstanceCanStartAtElementType[Scenario[type=CALL_ACTIVITY, modelInstance=io.camunda.zeebe.model.bpmn.impl.BpmnModelInstanceImpl@105cf4b, variables={}]]
…

♻️ This comment has been updated with latest results.

korthout added 8 commits March 7, 2023 19:12
I want to add a parameter to the constructor, but discovered there was
this unused one as well. To reduce the effort, I just deleted this one.

The unused constructor was keeping the typedRecordProcessorFactory from
being final. So, now it can be made final as well.

(cherry picked from commit 76e13ec)
The EngineCfg will be used for users to configure the engine at a system
level. It does not allow to configure the different engines per
partition. All configurations will be applied to the engine of each
partition.

This requires changes in both the broker and the engine. The engine does
not have a dependency on the broker. So the EngineCfg needs to be mapped
to an EngineConfig that is specified by the workflow engine.

(cherry picked from commit 63dcfb9)

Conflicts:
	broker/src/main/java/io/camunda/zeebe/broker/system/partitions/impl/steps/StreamProcessorTransitionStep.java
	engine/src/test/java/io/camunda/zeebe/engine/util/TestStreams.java
Adds the the `broker.engine.messages` configuration, allowing to
configure the:
- `ttlCheckerBatchLimit`
- `ttlCheckerInterval`

(cherry picked from commit 271c221)
We're not yet sure whether we want to expose these configuration options
going forward. We may also move the recent changes into feature flags,
which would make those changes experimental as well. It makes only sense
to move the entire config settings for this into the experimental
section.

(cherry picked from commit 31cbbf8)

Conflicts:
	broker/src/main/java/io/camunda/zeebe/broker/system/configuration/ExperimentalCfg.java
	broker/src/main/java/io/camunda/zeebe/broker/system/partitions/impl/steps/StreamProcessorTransitionStep.java
This enables the experimental configuration settings:
- zeebe.broker.experimental.engine.messages.ttlCheckerBatchLimit
- zeebe.broker.experimental.engine.messages.ttlCheckerInterval

These settings are now used by the MessageTimeToLiveChecker.

(cherry picked from commit b5d4d4a)

Conflicts:
	engine/src/main/java/io/camunda/zeebe/engine/processing/EngineProcessors.java
	engine/src/main/java/io/camunda/zeebe/engine/processing/streamprocessor/TypedRecordProcessorContext.java
	engine/src/main/java/io/camunda/zeebe/engine/processing/streamprocessor/TypedRecordProcessorContextImpl.java
	engine/src/test/java/io/camunda/zeebe/engine/processing/message/MessageStreamProcessorTest.java
The checker is run async, it is no longer running on the same
actor/thread

(cherry picked from commit bc3a7e6)
Running the message TTL Checker asynchronous is mostly untested. To
avoid unforseen situations (especially as we are releasing this in a
patch release), it should be disabled by default and can be enabled if
users want to try it out.

We can choose whether or not we want to enable this feature by default
with the upcoming 8.2 release.

(cherry picked from commit 7a2576a)

Conflicts:
	broker/src/main/java/io/camunda/zeebe/broker/system/configuration/FeatureFlagsCfg.java
	dist/src/main/config/broker.standalone.yaml.template
	dist/src/main/config/broker.yaml.template
	engine/src/main/java/io/camunda/zeebe/engine/processing/message/MessageEventProcessors.java
	engine/src/test/java/io/camunda/zeebe/engine/processing/message/MessageStreamProcessorTest.java
	util/src/main/java/io/camunda/zeebe/util/FeatureFlags.java
	util/src/test/java/io/camunda/zeebe/util/FeatureFlagsTest.java
This means the behavior is effectively equivalent and user space is
unaffected (unless there are more than max int messages to expire).

We can adjust the default value in minor releases.

(cherry picked from commit 78b3349)
@korthout korthout force-pushed the forwardport-11947-to-main branch from b773400 to c2f562b Compare March 7, 2023 18:13
@korthout korthout marked this pull request as ready for review March 7, 2023 18:13
Copy link
Member

@romansmirnov romansmirnov left a comment

Choose a reason for hiding this comment

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

LGTM

@korthout
Copy link
Member Author

korthout commented Mar 8, 2023

Discussed with @oleschoenburg and looks good to merge

bors merge

ghost pushed a commit that referenced this pull request Mar 8, 2023
11956: [Forward main] Enable message time-to-live checker configuration r=korthout a=korthout

## Description

<!-- Please explain the changes you made here. -->
Forwards #11947 to `main`.

Conflicts were resolved by hand in multiple commits. Please review the commits with the `Conflicts:` section in the commit message with additional care.

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #11922



Co-authored-by: Nico Korthout <[email protected]>
@ghost
Copy link

ghost commented Mar 8, 2023

Build failed:

@korthout
Copy link
Member Author

korthout commented Mar 8, 2023

Infra problems with GH.

bors merge

@ghost
Copy link

ghost commented Mar 8, 2023

Build succeeded:

@ghost ghost merged commit 1b4ca08 into main Mar 8, 2023
@ghost ghost deleted the forwardport-11947-to-main branch March 8, 2023 15:49
This pull request was closed.
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.

Allow configuring the MessageTimeToLiveChecker
2 participants