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

refactor(engine): use hasProcessingReachedTheEnd to detect state #257

Merged
merged 5 commits into from
Apr 22, 2022

Conversation

pihme
Copy link
Contributor

@pihme pihme commented Mar 14, 2022

Description

Use hasProcessingReachedTheEnd to detect state

Related issues

closes #133

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

@github-actions
Copy link

github-actions bot commented Mar 14, 2022

Unit Test Results

  32 files    32 suites   1m 9s ⏱️
  98 tests   98 ✔️ 0 💤 0
307 runs  307 ✔️ 0 💤 0

Results for commit 824c909.

♻️ This comment has been updated with latest results.

@CLAassistant
Copy link

CLAassistant commented Mar 14, 2022

CLA assistant check
All committers have signed the CLA.

@pihme pihme force-pushed the 133-update-engine-state-monitor branch from 660dfa7 to b1621a4 Compare March 14, 2022 15:38
pihme added 4 commits April 13, 2022 16:47
Disable EngineStateMonitor test temporarily. The test has to be rewritten
With the previous logic, idle state callbacks were called immediately. On local machine I did not find any flaky tests, but on CI pipeline there was one.
The new logic waits a bit before notifying the idle state callbacks
@remcowesterhoud remcowesterhoud force-pushed the 133-update-engine-state-monitor branch from cf345b8 to 9eba709 Compare April 14, 2022 14:30
@remcowesterhoud
Copy link
Contributor

@pihme I finally found the cause of the testcontainer getting killed. I still don't know why it happens, but could you have a look at the latest commit and see what you think?

@pihme
Copy link
Contributor Author

pihme commented Apr 14, 2022

@remcowesterhoud Seems ok.

I wonder whether we can improve it to something like this:

    try {
      return streamProcessor.hasProcessingReachedTheEnd().join();
    } catch (final Exception e) {
      LOG.debug("Exception occurred while checking idle state", e);
      // A ExecutionException may be thrown here if the actor is already closed. For some mysterious
      // reason this causes the testcontainer to terminate, which is why we need to catch it.
      // We cannot catch the ExecutionException itself, as Zeebe turns this into an unchecked
      // exception. Because of this we need to catch Exception instead.
      return actor.isClosed();
    }

I would assume that a closed actor is equivalent to an idle state. This way we wouldn't stall callers when the actor is closed

Exceptions thrown in the TimerTask seem to terminate our testcontainer. Since checking if processing has reached the end occasionally throws ExecutionExceptions because the actor was already closed the testcontainer would terminate all the time, failing all other testcases. Catching this exception seems to have fixed the issue.
@remcowesterhoud remcowesterhoud force-pushed the 133-update-engine-state-monitor branch from 9eba709 to 824c909 Compare April 20, 2022 08:50
@remcowesterhoud remcowesterhoud marked this pull request as ready for review April 20, 2022 08:50
@remcowesterhoud
Copy link
Contributor

@korthout This one might be slightly complex to review, but as Peter and I both worked on it I think it'd be good if someone else reviews it.
If you need any help or extra explanation please let me know. I'll be happy to walk you through it.

@remcowesterhoud
Copy link
Contributor

@saig0 Since @korthout mentioned he was a bit swarmed at the moment, maybe you have time for this review instead 🙂

@remcowesterhoud remcowesterhoud requested review from saig0 and removed request for korthout April 20, 2022 13:38
Copy link
Member

@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.

@pihme / @remcowesterhoud LGTM 🚀

@remcowesterhoud remcowesterhoud merged commit f2d036b into main Apr 22, 2022
@remcowesterhoud remcowesterhoud deleted the 133-update-engine-state-monitor branch April 22, 2022 07:47
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.

Modify EngineStateMonitor to use built-in Zeebe functionality
4 participants