Fix performance regression in group::dump() #5002
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shaunrd0
approved these changes
May 23, 2024
|
I removed the test I ported to |
ypatia
force-pushed
the
yt/sc-48006/fix_group_dump_remote_issue
branch
from
fe5e2fe to
43778d0
Compare
KiterLuc
added a commit
that referenced
this pull request
May 27, 2024
After merging #5002 and #4999, dump for non existing group recursive group stopped working. This is because the error logic in 5002 expected a status code whilst 4999 changes to throw an exception. The fix changes the group code to throw a known exception which is caught at the upper level as a group not found exception. This is much cleaner that using a failed status code, which could mean many other issues. --- TYPE: NO_HISTORY DESC: Fix group not found issue.
KiterLuc
added a commit
that referenced
this pull request
May 27, 2024
After merging #5002 and #4999, dump for non existing group recursive group stopped working. This is because the error logic in 5002 expected a status code whilst 4999 changes to throw an exception. The fix changes the group code to throw a known exception which is caught at the upper level as a group not found exception. This is much cleaner that using a failed status code, which could mean many other issues. Also, taking advantage to unstatus group.cc/h. --- TYPE: NO_HISTORY DESC: Fix group not found issue.
Shelnutt2
changed the title
tiledb:// arrays
|
@ypatia this issue effects all backends. It's not specific in anyway to tiledb URIs. Refetching the type when we already have it as a means if checking if a URI exist is universally am issue and logically incorrect. |
You are right, I will adapt the PR and shortcut descriptions. The fix is however covering all the cases, so we are good. Please correct me if I am wrong and still missing something. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fix to 38499 , introduced a performance regression to
Group::dump, since now the method callsobject_typefor every member in a for loop while the type is already known and every such call initiates two extra checks (one foris_arrayand one foris_group).The fix in this PR is to let group open fail when the group is not there instead, check on the return value and log that the array doesn't exist.
TYPE: BUG
DESC: Fix performance regression in group::dump()