Skip to content

Conversation

@konekomind
Copy link
Contributor

-Cleans up pre flush and post flush actions when writing incidents into batch
-Fixes creation of boundary events on multi instances
-Fixes scenario when input for multi instance is empty
-Adds tests for multiInstance message publishing, cleanup of subscriptions when multiInstance completes and fixes above
-Adds logs to a flaky tests in hope of fixing it later

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @konekomind, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enhancing the stability and correctness of multi-instance processes, particularly around incident handling, boundary events, and message publishing. It also addresses edge cases such as empty input collections and includes additional testing and logging to improve reliability.

Highlights

  • Batch Incident Writing: Cleans up pre-flush and post-flush actions when writing incidents into a batch, ensuring a clean state for each incident.
  • Multi-Instance Boundary Events: Fixes the creation of boundary events on multi-instances, ensuring that events are correctly associated with their instances.
  • Empty Multi-Instance Input: Addresses a scenario where the input for a multi-instance is empty, handling the case gracefully without errors.
  • Multi-Instance Message Publishing: Adds tests for multi-instance message publishing, verifying the correct behavior of message events in multi-instance scenarios.
  • Subscription Cleanup: Ensures proper cleanup of subscriptions when a multi-instance completes, preventing resource leaks and maintaining system stability.
  • Flaky Test Logging: Adds logs to a flaky test in the hope of fixing it later, aiding in debugging and identifying the root cause of intermittent failures.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • internal/cluster/partition/config.go
    • Removed an unnecessary newline for code formatting.
  • pkg/bpmn/engine_batch.go
    • Initialized preFlushActions and postFlushActions slices in WriteTokenIncident and WriteMessageIncident functions to avoid nil pointer dereferences.
  • pkg/bpmn/events_test.go
    • Added new tests for message event multi-instance scenarios, including business rule, call activity, service task, sub-process, and parallel variations.
    • Added time import for tests.
    • Added logging to flaky tests.
  • pkg/bpmn/sub_process.go
    • Added boundary event subscription creation for active multi-instance activities.
    • Handled ActivityStateCompleted for multi instances.
    • Handled the case where input collection is empty.
  • pkg/bpmn/sub_process_test.go
    • Added assertions to check for zero active message subscriptions after process completion in various test cases.
    • Added a test case for skipping multi-instance when input is empty.
    • Added a test case for parallel multi-instance when input is empty.
    • Added logging to flaky tests.
  • pkg/bpmn/test-cases/call-activity-simple.bpmn
    • Added a boundary message event to the call activity process.
  • pkg/bpmn/test-cases/multi_instance_business_rule.bpmn
    • Updated zeebe subscription correlation key to be a string.
  • pkg/bpmn/test-cases/multi_instance_call_activity_task.bpmn
    • Updated zeebe subscription correlation key to be a string.
  • pkg/bpmn/test-cases/multi_instance_parallel_business_rule.bpmn
    • Updated zeebe subscription correlation key to be a string.
  • pkg/bpmn/test-cases/multi_instance_parallel_call_activity_task.bpmn
    • Updated zeebe subscription correlation key to be a string.
  • pkg/bpmn/test-cases/multi_instance_parallel_service_task.bpmn
    • Updated zeebe subscription correlation key to be a string.
    • Moved boundary event.
  • pkg/bpmn/test-cases/multi_instance_service_task.bpmn
    • Updated zeebe subscription correlation key to be a string.
    • Moved boundary event.
  • pkg/bpmn/test-cases/multi_instance_sub_process_task.bpmn
    • Updated zeebe subscription correlation key to be a string.
  • pkg/bpmn/test-cases/simple_sub_process_task.bpmn
    • Added a boundary message event to the sub process task.
    • Updated zeebe subscription correlation key to be a string.
Activity
  • The pull request includes fixes for multi-instance processes, addressing issues with boundary events, empty inputs, and subscription cleanup.
  • Tests have been added to verify the behavior of message events in multi-instance scenarios.
  • Logging has been added to a flaky test to aid in debugging.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several important fixes for multi-instance activities, including the correct creation of boundary events and handling of empty input collections. The accompanying tests are a valuable addition. However, I've identified a few areas for improvement in the new tests. Specifically, there's significant code duplication that could be addressed with table-driven tests, the use of time.Sleep which can cause flakiness, and a couple of test cases that appear to instantiate the wrong process. Additionally, some temporary debugging code has been committed which should be removed. My detailed comments provide suggestions to resolve these issues.

Comment on lines +828 to +857
//Logging the flaky tests
if counter > 5 {
for _, s := range engineStorage.ProcessInstances {
println(s.ProcessInstance().Key)
println(s.ProcessInstance().State.String())
println(s.ProcessInstance().VariableHolder.GetLocalVariable("testOutputCollection").(string))
println(s.ProcessInstance())
}
for _, t := range engineStorage.ExecutionTokens {
println(t.Key)
println(t.State.String())
println(t.ElementId)
}
}
counter++
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This block of code adds logging to debug a flaky test. Such debugging code should not be committed to the main branch. Please remove it before merging.

-Cleans up pre flush and post flush actions when writing incidents into batch
-Fixes creation of boundary events on multi instances
-Fixes scenario when input for multi instance is empty
-Adds tests for multiInstance message publishing, cleanup of subscriptions when multiInstance completes and fixes above
-Adds logs to a flaky tests in hope of fixing it later
@konekomind
Copy link
Contributor Author

/gemini review

gemini-code-assist[bot]

This comment was marked as duplicate.

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