Skip to content

AWS: Fix TestGlueCatalogTable failure#4355

Merged
jackye1995 merged 1 commit intoapache:masterfrom
xiaoxuandev:fix-tests
Mar 17, 2022
Merged

AWS: Fix TestGlueCatalogTable failure#4355
jackye1995 merged 1 commit intoapache:masterfrom
xiaoxuandev:fix-tests

Conversation

@xiaoxuandev
Copy link
Contributor

No description provided.

static final Map<String, String> tableLocationProperties = ImmutableMap.of(
TableProperties.WRITE_DATA_LOCATION, "s3://" + testBucketName + "/writeDataLoc",
TableProperties.WRITE_METADATA_LOCATION, "s3://" + testBucketName + "/writeMetaDataLoc",
TableProperties.WRITE_DATA_LOCATION, "s3://" + testBucketName + "/writeDataLoc",
Copy link
Contributor

@singhpk234 singhpk234 Mar 17, 2022

Choose a reason for hiding this comment

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

I would recommend changing this :

Assert.assertEquals("additionalLocations should match", tableLocationProperties.values(),

sort both the collections as it may happen post picking these changes it failing in my machine

    Assert.assertEquals("additionalLocations should match", tableLocationProperties.values().stream().sorted().collect(
        Collectors.toList()),
        response.table().storageDescriptor().additionalLocations().stream().sorted().collect(Collectors.toList()));

as I can confirm that for the present master branch these tests passes on my machine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. That makes sense as we are comparing a map's valueSet and a list. I have updated the change. Please have another look.

Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @xiaoxuandev for the fix :) !!

Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

also looks good to me!

Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

thanks for the fix!

@jackye1995 jackye1995 merged commit d2bb30f into apache:master Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants