Skip to content

Make GC work with EMR 7.0.0 #9013

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

Open
wants to merge 96 commits into
base: master
Choose a base branch
from
Open

Make GC work with EMR 7.0.0 #9013

wants to merge 96 commits into from

Conversation

idanovo
Copy link
Contributor

@idanovo idanovo commented May 4, 2025

No description provided.

Copy link

github-actions bot commented May 4, 2025

E2E Test Results - Quickstart

12 passed

Copy link

github-actions bot commented May 4, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed, 1 skipped

@idanovo idanovo had a problem deploying to Treeverse signing May 4, 2025 10:57 — with GitHub Actions Error
@idanovo idanovo had a problem deploying to Treeverse signing May 4, 2025 10:58 — with GitHub Actions Failure
@idanovo idanovo temporarily deployed to Treeverse signing May 4, 2025 10:58 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 4, 2025 11:12 — with GitHub Actions Inactive
@idanovo idanovo had a problem deploying to Treeverse signing May 4, 2025 11:13 — with GitHub Actions Failure
@idanovo idanovo temporarily deployed to Treeverse signing May 4, 2025 12:01 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 4, 2025 12:02 — with GitHub Actions Inactive
@idanovo idanovo had a problem deploying to Treeverse signing May 5, 2025 11:16 — with GitHub Actions Failure
@idanovo idanovo temporarily deployed to Treeverse signing May 5, 2025 11:16 — with GitHub Actions Inactive
@idanovo idanovo had a problem deploying to Treeverse signing May 5, 2025 12:12 — with GitHub Actions Failure
@idanovo idanovo temporarily deployed to Treeverse signing May 5, 2025 12:12 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 5, 2025 12:42 — with GitHub Actions Inactive
@idanovo idanovo had a problem deploying to Treeverse signing May 5, 2025 12:43 — with GitHub Actions Failure
@idanovo idanovo temporarily deployed to Treeverse signing May 5, 2025 12:46 — with GitHub Actions Inactive
@idanovo idanovo had a problem deploying to Treeverse signing May 5, 2025 12:46 — with GitHub Actions Failure
@idanovo idanovo temporarily deployed to Treeverse signing May 5, 2025 12:54 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 5, 2025 12:55 — with GitHub Actions Inactive
@@ -127,7 +132,7 @@ object StorageUtils {

private def initializeS3Client(
configuration: ClientConfiguration,
credentialsProvider: Option[AWSCredentialsProvider],
credentialsProvider: Option[Any],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to keep interface of the same type that builderWithEndpoint.withCredentials accepts instead of general object?

@idanovo idanovo had a problem deploying to Treeverse signing May 15, 2025 09:00 — with GitHub Actions Failure
@idanovo idanovo temporarily deployed to Treeverse signing May 15, 2025 09:00 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 15, 2025 09:06 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 15, 2025 09:06 — with GitHub Actions Inactive
@idanovo idanovo requested a review from nopcoder May 15, 2025 09:39
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

We pass AWSCredentialsProvider and still check instance type

@idanovo idanovo requested a review from nopcoder May 15, 2025 14:03
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

lgtm - minor comments:

  • log bad format message
  • some consider roleArn as sensitive information that should not be logged at default level.

}
} catch {
case e: io.treeverse.jpebble.BadFileFormatException =>
logger.error(s"File format validation failed for: ${gravelerSplit.path}", e)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit; failed to read sstable, bad file format

@idanovo idanovo temporarily deployed to Treeverse signing May 18, 2025 08:55 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 18, 2025 08:55 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 18, 2025 09:15 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 18, 2025 09:15 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 18, 2025 11:26 — with GitHub Actions Inactive
@idanovo idanovo temporarily deployed to Treeverse signing May 18, 2025 11:26 — with GitHub Actions Inactive
Copy link
Contributor

@Isan-Rivkin Isan-Rivkin left a comment

Choose a reason for hiding this comment

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

one comment, non blocker.
is this relevant?

}

// Check for Hadoop's assumed role configuration
val roleArn = System.getProperty("spark.hadoop.fs.s3a.assumed.role.arn")
Copy link
Contributor

Choose a reason for hiding this comment

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

please move to consts file we have

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make GC work with EMR 7.x.x
3 participants