-
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
Send baseMappingName when forwarding FullMeshRequest from TS to DS #7887
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.
The changes look good to me. I only found a very minor thing. You can decide whether my suggestion makes sense to apply.
I did not test as this is a rather complex scenario, but as you wrote already -> you already tested the changes :D
AgglomerateFileKey( | ||
organizationName, | ||
datasetName, | ||
dataLayerName, | ||
mappingName | ||
), |
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.
You have a duplication of this AgglomerateFileKey
constructer (see lines 69ff.). Prior to these changes there was only one definition of the constructor and its result was stored in a val agglomerateFileKey.
=> A minor suggestion from my side would be to restore this and only have code that constructs the AgglomerateFileKey
once :)
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 suggestion! Yes, I have already considered this. However, I ultimately decided against it, since the only ways that I saw to deduplicate this (without completely changing the logic again) would be either to construct the AgglomerateFileKey above the whole match statement, but that doesn’t seem right, since in most cases it won’t be used, or to extract it to another function, but that doesn’t seem right, because its call would look exactly the same as the direct constructor call.
So I tend to think in this case breaking the DRY-rule is less bad than the alternatives :)
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.
Sure, sounds like a good reason to me 👍
Fixes a rare bug that occurs when the fullMesh.stl route is used on an editable mapping, but for an agglomerate ID that has not been edited.
In this case, the tracingstore (TS) redirects the request to the datastore (DS), and sends the mapping name so that the datastore can use the hdf5 mapping file to look up the segment ids for the agglomerate. The bug was that in this case, the editable mapping id was sent as mapping name, rather than the base mapping name, which corresponds to the hdf5 file.
While fixing this, I also restructured the logic of the
MeshMappingHelper
to more explicitly check the different combinations of the optional mappingName and tracingId.Also added a few comments concerning baseMapping name in a few method signatures.
Steps to test:
Issues: