AWS: Use provided glue catalog id in defaultWarehouseLocation

[grbinho]

defaultWarehouseLocation call to glue.getDatabase did not set catalog id while building a request.
This results in software.amazon.awssdk.services.glue.model.EntityNotFoundException: Database my_database not found. error when table is being created in another AWS account.

More details are in #6215

@singhpk234
If you can take a look, it would be appreciated.
I did not add any tests for it since I saw that similar tests for other methods do not exist.

[singhpk234]

LGTM ! Thanks @grbinho .

You can add a UT for now it by mocking GetDatabase request and making sure catalogId passed in it. can ref this existing test

cc @jackye1995

[grbinho]

LGTM ! Thanks @grbinho .

You can add a UT for now it by mocking GetDatabase request and making sure catalogId passed in it. can ref this existing test

cc @jackye1995

I added a unit test with AwsProperties.GLUE_CATALOG_ID property set in AwsProperties.
This seemed like the way to go with this one since test is supposed to validate that property is actually used in the method.
I'm not that experienced with Java and Mockito, so feedback on the correct way to do this is welcome!

[ajantha-bhat]

The integration tests in TestGlueCatalogNamespace also don't set the catalogId, do we need to set it in those files also?

[grbinho]

Are there multiple AWS accounts available for integration tests?
I could not find that setup either in AwsIntegTestUtil or TestAssumeRoleAwsClientFactory (as and obvious potential cross account example).

[ajantha-bhat]

I presume it is a single account.
The Integration testcase can be executed by exporting the mentioned environment variables (#4855)

Thanks for the clarification. may be no need of catalog id for these tests.

[grbinho]

@ajantha-bhat Can you please do another quick check. I changed how AwsProperties are initialized in the test. New way looks cleaner to me.

[ajantha-bhat]

@ajantha-bhat Can you please do another quick check. I changed how AwsProperties are initialized in the test. New way looks cleaner to me.

LGTM

[grbinho]

@ajantha-bhat @singhpk234
Do you need anything else from me to get this merged?

[ajantha-bhat]

cc: @Fokko, @rdblue, @RussellSpitzer, @jackye1995

[JonasJ-ap]

LGTM!

Additional note: I checked other request builders in GlueCatalog and GlueTableOperations and found that these two requests also not include the catalogId:
I wonder if we shall also add catalogId to these two requests (may be in another PR)

CreateTableRequest in:

iceberg/aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java

Lines 221 to 230 in 4caf1b4

	private boolean createGlueTempTableIfNecessary(TableMetadata base, String metadataLocation) {
	if (awsProperties.glueLakeFormationEnabled() && base == null) {
	// LakeFormation credential require TableArn as input，so creating a dummy table
	// beforehand for create table scenario
	glue.createTable(
	CreateTableRequest.builder()
	.databaseName(databaseName)
	.tableInput(
	TableInput.builder()

DeleteTableRequest in

iceberg/aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java

Lines 243 to 249 in 4caf1b4

	private void cleanupGlueTempTableIfNecessary(
	boolean glueTempTableCreated, CommitStatus commitStatus) {
	if (glueTempTableCreated && commitStatus != CommitStatus.SUCCESS) {
	glue.deleteTable(
	DeleteTableRequest.builder().databaseName(databaseName).name(tableName).build());
	}
	}

[grbinho]

I wonder if we shall also add catalogId to these two requests (may be in another PR)

For createGlueTempTableIfNecessary I'm not sure that is necessary since it's talking about Lake Formation.
Usually with Lake Formation cross account access is handled by Lake Formation (and resource shares).
In normal deployments tables would always be created in your "local" catalog and then shared with others and/or central catalog.

That said, I'm not that familiar with the code base enough to be sure it's unnecessary.

Maybe @amogh-jahagirdar knows?

[grbinho]

@JonasJ-ap @ajantha-bhat
Hi guys, can you advise how we can move this MR forward?

[rdblue]

Running CI.

[rdblue]

Thanks, @grbinho!