Skip to content
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 integration tests for out_s3 #75

Merged
merged 5 commits into from
Oct 13, 2020
Merged

Conversation

PettitWesley
Copy link
Contributor

These can't be merged until out_s3 is released.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@PettitWesley PettitWesley requested a review from a team as a code owner September 18, 2020 04:54
@@ -121,6 +131,9 @@ func validate(s3Client *s3.S3, response *s3.ListObjectsV2Output, bucket string,
if d == "" {
continue
}
if len(d) > 500 {
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing something. Why do we need this condition and the upper limit is 500?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the new S3 logger, it spits out log lines that are just numbers, and also large lines of 1KB random data (because S3 can only send data in chunks of a few MB). I wanted to make it so that the code didn't waste time trying to process those lines with random data.

500 is an arbitrary number between the size of each type of log event. The integer lines that we count will be smaller and the random data lines will be larger.

# intermixed with 1 KB lines of random data
# these logs are written over the course of a little over 1 minute
# because we want to test S3's local buffering
# Why 7717? Its a prime number. And I like that.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol- I was searching for it.

@hossain-rayhan
Copy link
Contributor

Overall looks good to me. Just one point- do we need to do anything specific for parallel testing (AMD64 and ARM64). I remember we had an issue with CloudWatch test. I don't think so because its using the same validation method as Kinesis and Firehose plugin.

@PettitWesley
Copy link
Contributor Author

Just one point- do we need to do anything specific for parallel testing (AMD64 and ARM64).

Do the arm changes run the integ tests in parallel? I thought it was just build. @meghnapr

If the tests will be run in parallel I will have to change the S3 key prefix in the test so that its different for each achitecture.

@MeghnaPrabhu
Copy link
Contributor

Yes, we are running integ tests in parallel so the S3 key prefix needs to be appended with the architecture.

@PettitWesley
Copy link
Contributor Author

Ive add the architecture in the S3 key so that this will work with arm64 support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants