AWS: Glue catalog strip trailing slash on DB URI

[amogh-jahagirdar]

Currently the Glue catalog does not strip the trailing slash on default DB location this can lead to path issues as seen in the below issue.

Related to: databricks/iceberg-kafka-connect#127

[amogh-jahagirdar]

cc @bryanck @okayhooni I just took another look at the issue and see that the DB URI s3://test_bucket/temp_beta does not have a trailing slash, so maybe my fix is insufficient. Can you confirm that in Glue your default DB location does not have a trailing slash?

In the end, I think we'll still need this fix because without it there's double slash possibilities but I would need to dig further to find the root cause for the issue's specific case

[dramaticlly]

looks like same response was used in method loadNamespaceMetadata (Line 518) as well, do we need to strip there as well?

[nastra]

I agree, I think we'd need to do the same in loadNamespaceMetadata()

[amogh-jahagirdar]

At first I was worried about the behavior change for callers of the API (now they cannot expect a trailing slash in case they were using the location in the response), but overall changing loadNamespaceMetadata makes sense. The API is pretty much used when updating namespace properties in which case stripping the trailing slash can serve as a correction mechanism for any future tables in the namespace. Existing tables in the namespace wouldn't see a change in behavior. Updated

[nastra]

Just a wild guess, but could it be possible that we're missing to strip a trailing slash in

iceberg/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

Line 232 in 81bf8d3

return String.format("%s/%s/%s", metadata.location(), METADATA_FOLDER_NAME, filename);

?

[amogh-jahagirdar]

Just a wild guess, but could it be possible that we're missing to strip a trailing slash in

iceberg/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

Line 232 in 81bf8d3

return String.format("%s/%s/%s", metadata.location(), METADATA_FOLDER_NAME, filename);

?

That's another place I spotted as well, unfortunately while I think we should have stripped the trailing slash there to begin with, I don't think we can change that. The reason being, is if we change that the metadataFileLocation path for new metadata files would be different compared to the original location which would have double slash. This difference in where the metadata file gets written may confuse users and also have unintended side effects for external systems. Also I need to step through the code a bit more, but not sure if we always guaranteed at build/write time of the location that the trailing slash would be stripped. In that case we would already be covered.

[amogh-jahagirdar]

Thanks @bryanck @dramaticlly @singhpk234 @nastra for reviews! @okayhooni feel free to comment on this PR if the db location in your case did not have a trailing slash. In that case there's still some more cases we'll need to look into.

[okayhooni]

@amogh-jahagirdar

Sorry for the late response and thanks to handle this issue..!

Yes, there is a trailing slash on the database location, as you said..! so, your commit will handle the issue..! thank you!