Repository navigation
Publish commits before notifying the change feed, and fix CI - #49
Merged
Merged
Conversation
The flusher appended the commit event to the feed and only then made the generation current, so a subscriber could react to generation g while new transactions still saw g - 1. Continuous views then looked up atoms that weren't visible yet and skipped their changes for good. Publishing first means a subscriber can always read the commit it is told about.
Four subscribers watch 500 commits and record any event whose generation is ahead of the current one. Against the old ordering it caught the race in 5 of 6 runs.
refresh replayed events up to the feed's last generation but checked tenants against the writer's snapshot, so a commit landing in between could still be skipped. Events past the snapshot are left for the next notification, as SemanticPlane already does.
For a merged pull request github.ref is main, so every merge shared one concurrency group and cancelled the previous merge's native and docker jobs. Group by pull request instead. A failed cache upload no longer fails a docker build that otherwise succeeded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI has been failing or getting cancelled after most merges since #25. Three separate causes.
1. A real race in the engine (the
buildfailures).SecurityTest.materializedViewsStayInsideTheirTenantfailed on 6 of the last 12 merges, always at the 5 s deadline. The group-commit flusher appended a commit to the change feed before making it the current generation. A subscriber could react to generation g while new transactions still saw g - 1. Since #25, continuous views look up each changed atom's tenant in their own snapshot. A not-yet-visible edge was skipped, and the view still advanced past it, so the change was lost from the view for good. It shows up on 2-core runners and almost never on a fast machine.currentfirst, then appends to the feed.MaterializedViews.refreshreplays only up tomin(feed.lastGeneration(), own snapshot). That closes the narrower case where a later commit lands between the writer starting and the refresh. This is whatSemanticPlanealready does.docs/storage/change-feed.mddescribed the old order as deliberate. I checked every place that combines a snapshot with the feed, and none depends on it. The doc now states the new guarantee: a subscriber can always read the commit it's told about.EngineTest.subscribersSeeTheCommitTheyAreToldAbout: four subscribers watch 500 commits. Against the old ordering it failed in 5 of 6 runs; with the fix it passed every run.2. Merges cancelling each other (the
cancelledruns). For a merged PR,github.refismain, so every merge shared one concurrency group and cancelled the previous merge's native and docker jobs. Grouped by PR number now.3. Docker cache saves (the
dockerfailures).failed to reserve cachewhen overlapping runs wrote the same cache entry. The image itself had built fine every time.cache-tonow hasignore-error=true, here and in the release workflow.Also includes the README tweak that drops the venue from the paper reference.
Full
./mvnw installpasses locally (92 tests), and each commit compiles on its own.