-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add missing fields in DynamoDBToS3Operator system test #31197
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
Conversation
| file_size=1000, | ||
| s3_bucket_name=bucket_name, | ||
| export_time=datetime(year=2023, month=4, day=10), | ||
| export_time=datetime.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.
Is this change significant?
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.
Yes, because in system tests we just want to make sure the DynamoDBToS3Operator is successfully able to call export_table_to_point_in_time with the minimum time window. So the exported data is small. But I'm open to any suggestions.
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.
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.
As long as we don't get weird API behaviour with a relatively instantaneous export_time seems fine to me. I'm assuming you've tested this?
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.
Yes, I have tested it on local and I have been developing using datetime.now() only, no issues so far.
| s3_bucket_name=bucket_name, | ||
| export_time=datetime(year=2023, month=4, day=10), | ||
| export_time=datetime.now(), | ||
| s3_key_prefix=f"{S3_KEY_PREFIX}-3-", |
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.
Why are we hard-coding the -3- part?
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.
No reason was just following other examples in the file
I think it's good to have it since it isolates this test case from other test cases in the file. Otherwise, there can be some side-effects if two testcase uses the same prefix. WDYT?
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 can get behind that reasoning. I didn't see the other two in the github "changes" diff, so I didn't catch the pattern.
|
I re-triggered the tests since the wait for ci images timed out. |
ferruzzi
left a comment
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.
Third time's the charm? :P
|
negative. |
|
@ferruzzi I'm not able to reproduce this on my local below code is working for me. I'm using breeze to run my test which is using docker image - Can you please share the details of environment in which you are running tests? I'll try to reproduce this on my local. |
|
Our environment is using Breeze in order to run system tests. We are building the last image with But given the exception I dont think it is question of breeze, the service Dynamodb is complaining that |
Oh yeah. I've seen it happen in the past. |
|
@vincbeck I think the datetime.now() return date which is not timezone aware, so for me in India - IST this will always give time which is ahead of UTC and If the export API assumes UTC time, it should have failed for me every time(which it didn't), right? or I'm missing something? |

There was a missing
s3_key_prefixin the system test forDynamoDBToS3Operatorand theexport_timedate format was not correct.