-
Notifications
You must be signed in to change notification settings - Fork 445
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
index: add receive hook #1644
base: master
Are you sure you want to change the base?
index: add receive hook #1644
Conversation
6caa695
to
4463073
Compare
Windows failures seem genuine, but i dont understand them yet. Edit:
ignoring EACCESS kinda fixes the issue, im not sure what happens though, something may have a handle to the file open while somethign else attempts the rename. |
edc931b
to
36edc4a
Compare
This removes the need for the `cond` storage and hooks up the index and its backing storage using the blobhub. Also fixed a bug where serverinit installs a wrapper instead of the storage which causes the receive hooks to be fired for the wrapper but not for the storage itself causing us to mix sync and index for it.
36edc4a
to
8c9aafc
Compare
@@ -28,9 +28,9 @@ import ( | |||
|
|||
const maxRemovesPerRequest = 1000 | |||
|
|||
func CreateRemoveHandler(storage blobserver.Storage) http.Handler { | |||
func CreateRemoveHandler(storage blobserver.Storage, config *blobserver.Config) http.Handler { |
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.
add docs to these methods? how's this config used? send separately?
@@ -29,22 +29,17 @@ import ( | |||
"perkeep.org/pkg/blobserver/protocol" | |||
) | |||
|
|||
func CreateStatHandler(storage blobserver.BlobStatter) http.Handler { | |||
func CreateStatHandler(storage blobserver.BlobStatter, config *blobserver.Config) http.Handler { |
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.
same
@@ -35,13 +36,13 @@ type BlobHub interface { | |||
// | |||
// If any synchronous receive hooks are registered, they're run before | |||
// NotifyBlobReceived returns and their error is returned. | |||
NotifyBlobReceived(blob.SizedRef) error | |||
NotifyBlobReceived(context.Context, blob.SizedRef) 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.
send all this context plumbing in its own PR(s)?
// NotifyBlobReceived will only return one of the errors. | ||
AddReceiveHook(func(blob.SizedRef) error) | ||
AddReceiveHook(func(context.Context, blob.SizedRef) 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.
and some docs on how the context is used?
found := false | ||
for _, errno := range []error{ | ||
syscall.ERROR_ALREADY_EXISTS, | ||
syscall.ERROR_ACCESS_DENIED, | ||
} { | ||
if errors.Is(linkErr, errno) { | ||
found = true | ||
break | ||
} | ||
} | ||
if !found { | ||
return err | ||
} |
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.
send separately with a test? document/justify the access denied on rename thing.
This removes the need for the
cond
+replica
storage configuration and hooks up the index and its backing storage using the blobhub. Also fixed a bug where serverinit installs a wrapper instead of the storage which causes the receive hooks to be fired for the wrapper but not for the storage itself causing us to mix sync and index for it.