Skip to content

Suggestion: acquire_writer() options object with notify_observers defaulting to true #63

Description

@yokuze

Suggestion

Give DatabaseWrapper::acquire_writer() an options object whose notify_observers
field defaults to true, so the observing writer is what a caller gets by default and
opting out is explicit at the call site.

pub struct AcquireWriterOptions {
   /// Route the commit through the change observer. Default: `true`.
   pub notify_observers: bool,
}

impl Default for AcquireWriterOptions {
   fn default() -> Self {
      Self { notify_observers: true }
   }
}
// unchanged, and still the observing writer
wrapper.acquire_writer().await?

// opting out says so, in the words that describe what happens
wrapper.acquire_writer_with(AcquireWriterOptions { notify_observers: false }).await?

Why

Today the choice is carried by two method names that do not say what separates them:

wrapper.acquire_writer()          // routes the commit through the change observer
wrapper.acquire_regular_writer()  // silently bypasses it

"Regular" reads as the ordinary one, which is the opposite of how the two should be
chosen. A caller should almost always want the writer that notifies observers. Nothing in
the word "regular" conveys "this write will not notify anyone", so the distinction has to
be carried by documentation instead of by the API.

The consequence is quiet: a write on the bypassing spelling commits normally and never
notifies, so a subscriber that depends on it does not update. There is a tracing::warn!
when observation is enabled, but as the doc comment notes, it compiles out in release
builds because the workspace pins tracing with release_max_level_off.

A named field beats a name for two reasons. notify_observers: false states the effect
rather than implying a category, and the default lives in one place instead of in whichever
method name a caller happens to reach for. It also leaves room for later per-acquisition
options without another method.

One open question on this shape: acquire_writer() and acquire_regular_writer()
currently return different guard types, so a single entry point parameterized at runtime
needs either a unified guard or a separate _with method as sketched above. Which of those
is right is your call.

Alternate suggestion

If an options object is more churn than it is worth, renaming the bypass alone would fix
the naming problem on its own:

Today Renamed
acquire_writer() unchanged
acquire_writer_with_attached(specs) unchanged
acquire_regular_writer() acquire_writer_without_observers()

That keeps the common path at the shortest name, changes no existing caller, and moves only
the bypass to a name that states what it does.

Notes

We have no call sites of acquire_regular_writer() ourselves, so this is not blocking
anything for us. It came out of a code review where the first question asked was "what is a
regular writer versus just acquire_writer?", which seemed worth passing upstream.

Happy to open a PR for whichever direction looks right.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions