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.
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
Add data loader core key and column utils #1771
base: master
Are you sure you want to change the base?
Add data loader core key and column utils #1771
Changes from 6 commits
2e38741
3797510
637b474
a113450
89028b4
0c34139
9c24445
79fee4b
4039083
2af371a
ee67ef7
d06c3b5
57738d3
c451948
d12526c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This doesn't seem to be used?
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.
My bad. It has become unused due to refactoring the error handling based on Toshi's feedback. I have removed it now.
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.
Adding
@Nullable
for nullable parameters would be helpful: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.
Added.
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.
I think we should set the cause of the exceptions:
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.
Added. Although to fix this I had to revise the used exceptions a bit.
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.
I'm wondering what if there are 3 tables that have
name
column and this exception is thrown for the second table. Users may want to know which table...? What do you think?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.
Updated the method and now passing in the table name as well. I had to add it because the table name is not part of the TableMetadata.
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.
@ypeckstadt Thanks for improving the error message!
But, sorry, I should've noticed this when posting the previous comment, other error messages (e.g., DATA_LOADER_INVALID_COLUMN_NON_EXISTENT) have the same issue. Also, the namespace name should be added as well. I think we have 2 options as follows:
KeyUtils
. Instead, we'll catch exceptions fromKeyUtils
and wrap it with the namespace and table names like this:I think either is fine. What do you think?
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.
I have made changes based on the above feedback. I went for option 1 as it seems like the safest solution. Option 2 is good as well, but then it shifts the responsibility for proper exception handling and logging to all classes that use the Key Utils. So it creates the risk that some classes do it and some don't.
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 improvement!
It looks like there is room to improve the other error messages from the same perspective (i.e., missing information about namespace and table). What do you think?
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.
@komamitsu My apologies for the late reply. (had to focus on ScalarFlow more). I have updated the PR and ensured each exception message includes the namespace and table name.
To avoid having to pass in all 3 with all methods, I have added a wrapper class called
ColumnInfo
that includes all 3 fields.