-
Notifications
You must be signed in to change notification settings - Fork 112
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
Selectable propagators #4070
Selectable propagators #4070
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some questions and nitpicks
// PathLookup defines the interface for the lookup component | ||
type PathLookup interface { | ||
NodeFromResource(ctx context.Context, ref *provider.Reference) (*node.Node, error) | ||
NodeFromID(ctx context.Context, id *provider.ResourceId) (n *node.Node, err error) | ||
|
||
InternalRoot() string | ||
InternalPath(spaceID, nodeID string) string | ||
Path(ctx context.Context, n *node.Node, hasPermission node.PermissionFunc) (path string, err error) | ||
MetadataBackend() metadata.Backend | ||
ReadBlobSizeAttr(ctx context.Context, path string) (int64, error) | ||
ReadBlobIDAttr(ctx context.Context, path string) (string, error) | ||
TypeFromPath(ctx context.Context, path string) provider.ResourceType | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of the interface entirely? I don't see a reason for keeping it - Reimplementing such a big interface will not be done anyways...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. But it's not only about enabling reimplementations but also about decoupling dependencies (just removing it currently leads to import cycles) and increasing testability (albeit I'm not sure whether the latter is the case here.
We can take a look at that, the lookup interface has been a nuisance a few times already, but I'd rather we do that as a separate change as it's gonna be a bigger refactoring.
} | ||
} | ||
|
||
func (p SyncPropagator) Propagate(ctx context.Context, n *node.Node, sizeDiff int64) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse the Propagate
method from the sync propagator in the async propagator? Or at least extract some parts of it to a common function. I can see lots of duplication in these two methods....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew... I just tried but there are lots of subtle changes and slightly different dependencies. I would suggest we defer this refactoring to a later point in time so we can get the feature in for testing now.
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
That allows for easier detection of stale processing dirs
Co-authored-by: kobergj <[email protected]>
d9e6086
to
344b222
Compare
This PR adds an experimental asynchronous propagator which propagates changes asynchronously outside of the request scope. It also combines changes that happen shortly after each other into one change.
The propagator can be configured, the original
sync
propagator is still the default.