chore(engine): add onFailed stream processor lifecycle #4281
Conversation
| try { | ||
| asyncSnapshotDirector.closeAsync().join(); | ||
| } catch (final Exception e) { | ||
| Loggers.IO_LOGGER.debug("Close snapshot director failed", e); |
There was a problem hiding this comment.
When we try to close asyncSnapshotDirector, Stream processor actor can be already closed because it has failed. AsynsSnapshotDirector tries to get the lastCommitPosition but fails because StreamProcessor is closed/failed.
There was a problem hiding this comment.
this is no longer true due to removing the enforcing snapshot
| assertThat(failedLatch.await(1000, TimeUnit.MILLISECONDS)).isTrue(); | ||
|
|
||
| // then | ||
| verify(lifecycleAware, times(1)).onRecovered(any()); |
There was a problem hiding this comment.
do we need the mock since we already have the latch with the other listener ?
There was a problem hiding this comment.
Found the mock useful to verify that onClose() is never called.
There was a problem hiding this comment.
Yes I saw that, but actually we should avoid to test something which should not happen, because this is often error prone and hard to test. I mean how long do you want to wait to ensure that it never happens :) It makes also hard to change things later. We should more focus on things we can verify. I really like the answer https://softwareengineering.stackexchange.com/a/345664/182290 on a related question
There was a problem hiding this comment.
Make sense. But if we don't test it, what prevents me from accidentally invoking listener:onClose() when the actor failed? Isn't part of the contract that either listener.onFailed or listener.onClose is invoked but not both?
There was a problem hiding this comment.
Actually nothing. But the question is also should the test prevent you from such thing? When the end result is the same it should be fine right? No matter how many or what kind of methods are called. Is it really part of the contract ? Does it need to be? What values gives us this. As I said we should try to avoid these kind of couplings, since it makes harder to add new features.
I know I also did this sometimes, since sometimes we have the feeling that we have to make sure such thing doesn't happen. But it makes things often more complicate then it needs to be. We should try to focus on things we can guarantee and verify without using mocks. Like I put a into a function and get b, but I don't care how it is calculated. On these listeners tests it is a bit harder I know.
But what we can do is just test things which should happen. Like if something fails it should call onFailure. When I close it, it should call onClose.
deepthidevaki
force-pushed
the
4081-onfail-listener
branch
from
76020d3
to
0344a91
Compare
deepthidevaki
force-pushed
the
4081-onfail-listener
branch
from
0344a91
to
7b24e78
Compare
deepthidevaki
force-pushed
the
4081-onfail-listener
branch
from
7b24e78
to
f1b9d16
Compare
|
bors r+ |
Build succeeded |
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Description
Enable stream processor lifecycle listeners to react when StreamProcessor fails.
Related issues
closes #4081
Pull Request Checklist
mvn clean install -DskipTestslocally before committing