-
Notifications
You must be signed in to change notification settings - Fork 24
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
Introduce data vault as storage backend abstraction #6899
Conversation
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.
Amazing stuff, props for getting through this so quickly, and even including tests :) Very much enjoying the diff line count as well 😆 This already feels a lot more designed and less patched together, compared to using bits and pieces of the NIO providers.
I added some suggestions. Most are fairly small, but one thing I think is interesting: whether or not to pass an additional key
to the actual read calls, or whether the key can itself be mapped to the VaultPath instances. Maybe you could read through my coments and have a look at that. I’m open to discussing this, it is well possible that I missed some cases that make the current version necessary.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/FileSystemsHolder.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/FileSystemsHolder.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/VaultPath.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/NullDataVault.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3Utilities.scala
Outdated
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3Utilities.scala
Outdated
Show resolved
Hide resolved
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.
Very nice cleanup! We are getting very close :)
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.
Looks good and works for me :)
I see that the ”Removed dev-only changes” checkbox is still open, is there something left? If so, could you clear that up? Then this should be ready to go
…il-notification * 'master' of github.com:scalableminds/webknossos: Update screenshots (#6934) Support rendering negative floats (#6895) Fix loading of webworkers in dev mode (#6933) Restore cache buster for webworkers (#6932) Introduce data vault as storage backend abstraction (#6899) Fix download button for annotations when tiff export is disabled (#6931)
…wings * 'master' of github.com:scalableminds/webknossos: Update screenshots (#6934) Support rendering negative floats (#6895) Fix loading of webworkers in dev mode (#6933) Restore cache buster for webworkers (#6932) Introduce data vault as storage backend abstraction (#6899) Fix download button for annotations when tiff export is disabled (#6931) Update PULL_REQUEST_TEMPLATE.md Prepare multi modality support (#6748) Improvements for terms-of-services modal (#6930) Fix creating task types with preferred mode (#6928)
URL of deployed dev instance (used for testing):
Steps to test:
TODOS
Name
Questions
Issues:
(Please delete unneeded items, merge only when none are left open)