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

reconciler.BatchOperations provides object revision, where reconciler.Operations does not #56

Open
gandro opened this issue Sep 25, 2024 · 1 comment

Comments

@gandro
Copy link
Member

gandro commented Sep 25, 2024

Consider the reconciler operations interfaces (as of v0.2.x):

type Operations[Obj any] interface {
	Update(ctx context.Context, txn statedb.ReadTxn, obj Obj) error
	Delete(context.Context, statedb.ReadTxn, Obj) error
	Prune(context.Context, statedb.ReadTxn, statedb.Iterator[Obj]) error
}

type BatchEntry[Obj any] struct {
	Object   Obj
	Revision statedb.Revision
	Result   error

	// ... unexported fields
}

type BatchOperations[Obj any] interface {
	UpdateBatch(ctx context.Context, txn statedb.ReadTxn, batch []BatchEntry[Obj])
	DeleteBatch(context.Context, statedb.ReadTxn, []BatchEntry[Obj])
}

Implementors of the batch are able to obtain the object revision, where as implementers of the single operations interface are not. Since the Operations interface is mandatory, clients might want to implement part of the Operations interface on top of the BatchOperations interface (basically issuing batches containing a single element). But this is currently not possible, because invoking BatchOperations requires a revision not given to the Operations implementation.

Thus, this issue suggests that the Operations methods also get a StateDB revision for each passed object.

@joamaki
Copy link
Contributor

joamaki commented Oct 3, 2024

Agree with Operations also getting the revision. Let's accumulate some more API changes and release this as part of v0.4.

The BatchOperations interface I'm not too happy about as there's fair bit of allocations going on. This is why I also didn't do batch ops in pkg/bpf/ops_linux.go as there wasn't too much benefit to using a BPF batch op due to the inefficiency of the interface (less sys CPU time spent though...). I think I would like to revisit its design and see if we can make it work nicely with BPF map batch updates with minimal copying and allocations. Perhaps using iter.Seq2[Obj, *Result] to avoid the []BatchEntry slice allocation and on the BPF ops side having largeish buffers in sync.Pool that we reuse.

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

No branches or pull requests

2 participants