-
Notifications
You must be signed in to change notification settings - Fork 23
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
Separate DataStore and TracingStore #3281
Conversation
@jstriebel @normanrz do you know what needs to be done so that we can publish the tracingstore image to dockerhub? |
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.
@philippotto I reviewed the frontend part, very good work :)
If I misunderstood something with the datastore/tracingstore url, then nevermind, but I think there are some spots in the tests where the wrong one of the two is used.
Do we know whether having to send two requests instead of one when requesting fallback segmentation data hurts performance much? Did you notice anything?
MIGRATIONS.md
Outdated
@@ -6,12 +6,13 @@ User-facing changes are documented in the [changelog](CHANGELOG.md). | |||
|
|||
## Unreleased | |||
- Some config keys have changed, if you overwrite them in your setup, please adapt: the `oxalis` prefix is renamed to `webKnossos` so the new keys are `webKnossos.user.time.tracingPauseInSeconds`, `webKnossos.tasks.maxOpenPerUser`, `webKnossos.newOrganizationMailingList` as well as `datastore.webKnossos.uri`, `datastore.webKnossos.secured`, `datastore.webKnossos.pingIntervalMinutes` for the datastore. | |||
- There is now a separate module for the tracing store, the datastore is no longer responsible for saving tracings. This module can run as a standalone application, or as a module of webknossos locally. It is recommended that you chose the option that was previously also in place for datastores. In case of a standalone datastore, the local one needs to be disabled in application.conf: `tracingstore.enabled = false` and `play.modules.disabled += "com.scalableminds.braingames.datastore.TracingStoreModule` – and in either case, the adress of the tracingstore (localhost or remote) needs to be inserted in the db in `webknossos.tracingStores` |
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.
- Either always use tracing store or tracingstore
- webKnossos instead of webknossos
- choose instead of chose
- I would also add a period to the end of the last sentence :)
CHANGELOG.md
Outdated
@@ -18,6 +18,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.md). | |||
- The UI for editing experience domains of users was improved. [#3254](https://github.com/scalableminds/webknossos/pull/3254) | |||
- The tracing layout was changed to be more compact. [#3256](https://github.com/scalableminds/webknossos/pull/3256) | |||
- It is no longer possible to draw outside of a viewport with the brush tool in volume tracing. [#3283](https://github.com/scalableminds/webknossos/pull/3283) | |||
- There is now a separate tracingstore module, the datastore is no longer responsible for saving tracings |
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.
Please add a period and link this PR
@@ -729,6 +729,7 @@ class DataApi { | |||
const dataset = Store.getState().dataset; | |||
|
|||
return doWithToken(token => { | |||
// todo: use tracingStore maybe? |
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.
Is this todo still valid? The method description makes it look like the method can be used to download segmentation data as well? Would the request for the segmentation data need to go to the tracing- or the datastore? Or both? ^^ :/
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.
Thanks for the comment. @fm3 and I decided to clarify the api description so that the download only refers to the dataset (and not tracing). I'm not sure whether this api is used anyway. If users voice interest in downloading volume tracings, we can add another option. Merging is hopefully not necessary.
@@ -617,6 +617,7 @@ class DataApi { | |||
const dataset = Store.getState().dataset; | |||
|
|||
return doWithToken(token => { | |||
// todo: use tracingStore maybe? |
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.
See comment in api_latest.
@@ -150,7 +150,7 @@ async function sendUpdateActions(explorational, queue) { | |||
const skeletonTracingId = explorational.tracing.skeleton; | |||
if (skeletonTracingId == null) throw new Error("No skeleton tracing present."); | |||
return sendRequestWithToken( | |||
`${explorational.dataStore.url}/data/tracings/skeleton/${skeletonTracingId}/update?token=`, | |||
`${explorational.dataStore.url}/tracings/skeleton/${skeletonTracingId}/update?token=`, |
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.
Shouldn't this be the tracingstore url? But the e2e tests go through successfully - I'm confused ^^
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.
It's probably because both are the same, right? But the tracingstore url would be semantically correct.
.returns(Promise.resolve({ token: "token2" })); | ||
|
||
return requestWithFallback(layer, batch).then(([buffer1, buffer2]) => { | ||
// console.log("buffer1", buffer1); |
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.
These can probably be removed
@@ -66,7 +66,7 @@ test.beforeEach(t => { | |||
const tracingId = ANNOTATION.tracing[contentType]; | |||
|
|||
Request.receiveJSON | |||
.withArgs(`${ANNOTATION.dataStore.url}/data/tracings/${contentType}/${tracingId}`) | |||
.withArgs(`${ANNOTATION.dataStore.url}/tracings/${contentType}/${tracingId}`) |
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.
Shouldn't this also be the tracingstore url?
@@ -122,7 +122,7 @@ test("SaveSaga should send request to server", t => { | |||
expectValueDeepEqual( | |||
t, | |||
saga.next(DATASTORE_URL), | |||
call(sendRequestWithToken, `${DATASTORE_URL}/data/tracings/skeleton/1234567890/update?token=`, { | |||
call(sendRequestWithToken, `${DATASTORE_URL}/tracings/skeleton/1234567890/update?token=`, { |
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.
Rename DATASTORE_URL
to TRACINGSTORE_URL
?
I updated the changelog + migration guide. |
URL of deployed dev instance (used for testing):
Notes
data/
prefixTODO:
tracings/volume/:tracingId/data
expecting POST JSON body just like binary data loadingapi/tracingstore
Steps to test:
Issues:
[ ] Updated documentation if applicable