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

lockset: Import package #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

lockset: Import package #5

wants to merge 1 commit into from

Conversation

bobvawter
Copy link
Contributor

@bobvawter bobvawter commented Nov 6, 2024

This commit extracts the lockset package from cockroachdb/replicator at commit ee8e2894. There are some API modifications to generalize the concept of a Task and to generalize metrics collection.


This change is Reviewable

This commit extracts the lockset package from cockroachdb/replicator at commit
ee8e2894. There are some API modifications to generalize the concept of a Task
and to generalize metrics collection.
Copy link

@MattWhelan MattWhelan left a comment

Choose a reason for hiding this comment

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

I have questions about the hybrid recursion/iteration at the end of Executor.dispose. Otherwise, just some suggestions for additional comments to help a new user orient.

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @bobvawter and @BramGruneir)


lockset/queue.go line 56 at r1 (raw file):

		sync.RWMutex

		// These waiters are used to maintain a global ordering.

I don't understand the use of waiters here, as these don't seem to be instances of the waiter struct? Also below, in Dequeue and Enqueue. Was there a rename refactor at some point from waiter to entry?


lockset/queue.go line 160 at r1 (raw file):

// Enqueue returns true if the value is at the head of its key queues.
// It is an error to enqueue a value if it is already enqueued.
func (q *Queue[K, V]) Enqueue(keys []K, val V) (atHead bool, err error) {

I think it's worth emphasizing that atHead does not imply IsHead. There are two distinct concepts of readiness going on here: a value is ready when it's at the head of all its key queues, but there's also a global linked list with a different concept of "head".


lockset/executor.go line 146 at r1 (raw file):

// will be dequeued from the Executor, possibly leading to cascading
// callbacks.
func (e *Executor[K]) dispose(w *waiter[K], cancel bool) {

"dispose" doesn't necessarily imply task execution to me. It would help if that were called out explicitly in the doc comment. It's pretty central to what's going on here, and it takes a lot of source diving to figure out that this is what actually executes the task.


lockset/executor.go line 243 at r1 (raw file):

		}
		for _, unblocked := range next {
			e.dispose(unblocked, false)

Could you comment on the interaction between the local next, the recursion here, and the guarantees around task ordering? Are there cases where the local next has tasks that still need to run when this recursion returns? This also seems to depend on the once-ness of tasks for correctness. Which is fine, but maybe worth calling out.


lockset/lockset.go line 19 at r1 (raw file):

// Package lockset contains utilities for ordering access to
// potentially-overlapping resources.
package lockset

There are several moving parts here. It would be nice if there were package-level docs describing how they fit together. It's also not immediately obvious what properties this system shares with other async executors that the user may be familiar with, and what might be different.

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