keep projections running as long events are loading #257
Conversation
|
Can you add tests (that fail without the change)?
…On Wed, Apr 16, 2025, 18:44 Piotr Zając ***@***.***> wrote:
@basz <https://github.com/basz> @prolic <https://github.com/prolic>
please, review.
—
Reply to this email directly, view it on GitHub
<#257 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADAJPHN2ZYDSUQBVCZEJ6D2Z3FNDAVCNFSM6AAAAAB3JK4HT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJQHA4DGOBVGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
*unixslayer* left a comment (prooph/pdo-event-store#257)
<#257 (comment)>
@basz <https://github.com/basz> @prolic <https://github.com/prolic>
please, review.
—
Reply to this email directly, view it on GitHub
<#257 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADAJPHN2ZYDSUQBVCZEJ6D2Z3FNDAVCNFSM6AAAAAB3JK4HT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJQHA4DGOBVGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
unixslayer
force-pushed
the
keep-projection-running-as-long-events-are-loading
branch
3 times, most recently
from
0cb821a to
bd3a48b
Compare
|
@prolic done if merged, please also to release it. |
unixslayer
force-pushed
the
keep-projection-running-as-long-events-are-loading
branch
2 times, most recently
from
6e10d05 to
a44ce44
Compare
|
I can have a look later today. But am wondering if this should be considered a breaking change release. Even though it's a small bug fix it changes a side effect that consuming applications might rely on. Any advise on this? |
@basz this is somehow a follow-up to #246. For projections running with I don't consider this as a BC but more like a critical issue fix in the context of projection management. |
|
We are running all our projections with |
|
Any tests included inside files named *TestCase aren't executed it seems. (see with vendor/bin/phpunit --testdox) Don't know if this slipped through or is an deliberate choice, but a lot of tests never ran... (including yours) |
|
see |
|
I didn't even notice that. Normally I run my own tests locally and let the test suite run in CI. I think this is happening due to changes in PHPUnit itself. We've had that issue in the project some time ago when switching to a newer version. I can check that later. |
unixslayer
force-pushed
the
keep-projection-running-as-long-events-are-loading
branch
from
a44ce44 to
4b0921b
Compare
projections should keep running when: - runs with `keepRunning: true` due it awaits for new events - it did not reached end of the stream, e.g. in case of resetting projection status should indicate actual status of projection therefore, `idle` should be set if projection wasn't supposed to keep running or end of stream was reached
unixslayer
force-pushed
the
keep-projection-running-as-long-events-are-loading
branch
from
4b0921b to
4cc9e4f
Compare
unixslayer
changed the title
|
@basz this one is also ready for review and release ;) |
| function (array $state, Message $event) use ($projectionManager, $gapDetection, $parallelConnection): array { | ||
| if ($state['iteration'] === 1) { | ||
| Assertion::true($gapDetection->isRetrying()); | ||
| $parallelConnection->commit(); |
There was a problem hiding this comment.
I had to change a little bit logic in this test due now each projection will try to finish by itself
|
@basz sorry for pushing but is there any chance to have this reviewed by today? |
|
nice work. one thing i noticed without the changes to the PdoEventStoreReadModelProjector.php the tests never run to completion. I would expect a failing test in such a case. going to merge anyway, feel free to have a look at that. |
increase minimum version of PDOEventStore due prooph/pdo-event-store#257
3 participants
projections should keep running when:
keepRunning: truedue it awaits for new eventsprojection status should indicate actual status of projection therefore,
idleshould be set if projection wasn't supposed to keep running or end of stream was reached