-
Notifications
You must be signed in to change notification settings - Fork 63
Fix: dictionary changed size during iteration in GCSObjectMetadataClient #468
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
Changes from 2 commits
954a135
61a5c22
e6357e5
fed2512
4b53078
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,6 +140,16 @@ def test_get_patched_obj_metadata_with_nested_required_task_outputs(self): | |
got['__required_task_outputs'], '{"nested_task": {"nest": {"__gokart_task_name": "task1", "__gokart_output_path": "path/to/output1"}}}' | ||
) | ||
|
||
def test_adjust_gcs_metadata_limit_size_runtime_error(self): | ||
large_labels = {} | ||
for i in range(100): | ||
large_labels[f'key_{i}'] = 'x' * 1000 | ||
|
||
result = GCSObjectMetadataClient._adjust_gcs_metadata_limit_size(large_labels) | ||
|
||
total_size = sum(len(k.encode('utf-8')) + len(v.encode('utf-8')) for k, v in result.items()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When working with non-ASCII characters (such as Japanese etc.), it is necessary because the number of characters and the number of bytes do not match. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so. But, in this test case, is it need? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I apologize. I misunderstood it as production code. Since this test only needs to verify that no error occurs, it doesn't seem necessary. |
||
self.assertLessEqual(total_size, 8 * 1024) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you define a constant value in MAX_GCS_METADATA_SIZE: Final[int] = 8 * 1024 and refer here and L157 of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your review. I've fixed it! |
||
|
||
|
||
class TestGokartTask(unittest.TestCase): | ||
@patch.object(_DummyTaskOnKart, '_get_output_target') | ||
|
Uh oh!
There was an error while loading. Please reload this page.