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 Xapi_event #6306

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

contificate
Copy link
Contributor

A code clarification effort, consisting of:

  • Accumulate lists of event-related objects within a record, instead of a tuple. Using a record permits functional record update, which means we avoid citing lists we have not changed.
  • Remove 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.
  • Factor out a routine to convert (table, obj, time) entries into event records, defining the accumulation of add, mod, and del events in terms of it: message events remain special cased because their contents are not in the database. This avoids duplicated code.

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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
@contificate
Copy link
Contributor Author

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.

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this

ocaml/xapi/xapi_event.ml Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could define a max3

([], [], [], !last_generation)
tables
)
let events =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above the type is called acc - where I think events is better.

ocaml/xapi/xapi_event.ml Outdated Show resolved Hide resolved
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 <[email protected]>
@contificate contificate marked this pull request as ready for review February 18, 2025 14:36
@contificate
Copy link
Contributor Author

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.

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.

2 participants