Refactor Xapi_event #6306
Merged
Refactor Xapi_event #6306
Conversation
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
added 3 commits
February 14, 2025 13:02
In Xapi_event, events are accumulated by folding over the set of tables associated with a subscriber's subscription record. These events are mostly accumulated as lists within a tuple. There is no analogue of functional record update for tuples in OCaml. This means that the separate accumulations have to cite values they will not update. By introducing records, we can only cite the fields we actually change. Signed-off-by: Colin James <colin.barr@cloud.com>
In event accumulation for event.from, the code uses a mutable variable to thread a value through event accumulation. However, this value itself is accumulated in the fold: it gets larger for each matching database event that matches a subscription. To avoid the complexity in effectively having a global, mutable variable, we drop it and make it more evident when it changes: it is changed when no events are accumulated (by grab_range). In the case that no events are accumulated, but the deadline hasn't been reached, the code tries to collect events again. It is during a retry that the last_generation needs to be bumped, as it defines the starting point by which to query the database for recent and deleted objects. In short, if no suitable events were gleaned from matching database object records since a given point, there's no point starting from there again. Signed-off-by: Colin James <colin.barr@cloud.com>
In order to provide event records to subscribers, we must convert the accumulated events of the form (table, objref, time) into event records. The process of doing this is simple for objects in the database. The only difference is that deletion events do not provide a snapshot (as the object has been deleted). To avoid repeating ourselves, we define an "events_of" function that accumulates event records. The function takes an argument that specifies whether an attempt to provide a snapshot should be performed. The reification of events associated with messages - which are not stored in the database - is untouched. This relies on a callback instated elsewhere. Signed-off-by: Colin James <colin.barr@cloud.com>
|
Opening as draft until XenRT is stable enough for me to submit a BVT+BST suite. It goes without saying, but it's important that event-related code does not break. |
lindig
approved these changes
Feb 14, 2025
| let prepend_recent obj stat _ ({creates; mods; last; _} as entries) = | ||
| let Stat.{created; modified; deleted} = stat in | ||
| if Subscription.object_matches subs table obj then | ||
| let last = max last (max modified deleted) in |
| ([], [], [], !last_generation) | ||
| tables | ||
| ) | ||
| let events = |
There was a problem hiding this comment.
Above the type is called acc - where I think events is better.
contificate
commented
Feb 14, 2025
Further changes to turn tuples into records. Also partially uncurries `collect_events` to make its intended use as a fold more apparent. Signed-off-by: Colin James <colin.barr@cloud.com>
|
BVT+BST: 212829. All green so far, with a few still running. Done some manual testing within XenCenter and XO-Lite, which rely entirely on the event API for their operation. These changes are refactoring, they should introduce no semantic change. |
minglumlu
reviewed
Feb 20, 2025
minglumlu
reviewed
Feb 20, 2025
minglumlu
approved these changes
Feb 20, 2025
|
BVT+BST is all green, merging. |
contificate
pushed a commit
to contificate/xen-api
that referenced
this pull request
Mar 7, 2025
Signed-off-by: Colin James <colin.barr@cloud.com>
last-genius
pushed a commit
to last-genius/xen-api
that referenced
this pull request
Mar 10, 2025
Signed-off-by: Colin James <colin.barr@cloud.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 20, 2025
Same as #6306 but retaining the suppression of potential snapshot-related exceptions. Now, there is commentary, a citation of the internal CA ticket (from 2012), and a commit hash citing the introduction of the `try`-`with`. --- Clean BVT+BST (214804) and works as expected under the internal error reproduction script. These commits were cherry picked back to the top, but I am happy to squash them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A code clarification effort, consisting of:
last_generation: this mutable variable is updated in a few places and complicates the code. Instead of relying it on being in scope (and mutated in other places), we explicitly pass it in and then thread the update to its value through the retry loop.(table, obj, time)entries into event records, defining the accumulation ofadd,mod, anddelevents in terms of it: message events remain special cased because their contents are not in the database. This avoids duplicated code.