Skip to content

Fix flaky TestS3WrongRegionPicked.testS3WrongRegionSelection #14615

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.

Sign up for GitHub

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

Merged
Praveen2112 merged 1 commit into from
Oct 14, 2022
Merged

Fix flaky TestS3WrongRegionPicked.testS3WrongRegionSelection #14615

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestS3WrongRegionPicked.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,15 @@ public class TestS3WrongRegionPicked
public void testS3WrongRegionSelection()
throws Exception
{
try (HiveMinioDataLake dataLake = new HiveMinioDataLake("test-bucket").start();
// Bucket names are global so a unique one needs to be used.
String bucketName = "test-bucket" + randomTableSuffix();
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 Oct 13, 2022

Choose a reason for hiding this comment

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

Can we add a comment on why we need this random bucket name ?

Copy link
Copy Markdown
Member

@Dith3r Dith3r Oct 13, 2022

Choose a reason for hiding this comment

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

MinioClient has something which is called createdBuckets which is static. Event if we start new Minio container, this will fail to create bucket with same name.

Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 Oct 13, 2022

Choose a reason for hiding this comment

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

But in the new minio container, the bucket doesn't exist right ?

Copy link
Copy Markdown
Member

@Dith3r Dith3r Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In MinioClient class from package io.trino.testing.minio we have a set of created buckets (which is persistent and independent of minio container and its buckets). Maybe we should clean this set on close?

Edit: If we do this test in loop, same name error will be a thing that will come up as first. I don't know if this is a reason for being flaky.

Copy link
Copy Markdown
Member Author

@skrzypo987 skrzypo987 Oct 13, 2022

Choose a reason for hiding this comment

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

This MinioClient class is strictly for testing purposes, so forcing a unique bucket naming seems like a good idea. If I had thought about it in the first place, there would be no bug.

Copy link
Copy Markdown
Member

@Dith3r Dith3r Oct 13, 2022

Choose a reason for hiding this comment

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

I would look up on BackgroundHiveSplitLoader if it behaves correctly when stopped and loadSplits is ongoing.


try (HiveMinioDataLake dataLake = new HiveMinioDataLake(bucketName).start();
QueryRunner queryRunner = S3HiveQueryRunner.builder(dataLake)
.setHiveProperties(ImmutableMap.of("hive.s3.region", "eu-central-1")) // Different than the default one
.build()) {
String tableName = "s3_region_test_" + randomTableSuffix();
queryRunner.execute("CREATE TABLE default." + tableName + " (a int) WITH (external_location = 's3://test-bucket/" + tableName + "')");
queryRunner.execute("CREATE TABLE default." + tableName + " (a int) WITH (external_location = 's3://" + bucketName + "/" + tableName + "')");
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 Oct 13, 2022

Choose a reason for hiding this comment

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

nit: Can we useString.format

Copy link
Copy Markdown
Member Author

@skrzypo987 skrzypo987 Oct 13, 2022

Choose a reason for hiding this comment

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

I actually prefer concatenation for trivial cases.

assertThatThrownBy(() -> queryRunner.execute("SELECT * FROM default." + tableName))
.getRootCause()
.hasMessageContaining("Status Code: 400")
Expand Down