Skip to content

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Mar 20, 2025

No description provided.

@nastra nastra marked this pull request as draft March 20, 2025 18:16
@github-actions github-actions bot added the core label Mar 20, 2025
@nastra nastra force-pushed the creds-from-loadtable branch from 9bc2853 to 8db24df Compare March 20, 2025 18:32
@nastra nastra force-pushed the creds-from-loadtable branch from 8db24df to efe6592 Compare March 27, 2025 16:29
@nastra nastra force-pushed the creds-from-loadtable branch from efe6592 to 716a064 Compare March 27, 2025 16:31
@nastra nastra changed the title Core: Use credentials from LoadTableResponse if available Core: Pass storage credentials from LoadTableResponse to FileIO Mar 27, 2025
@nastra nastra force-pushed the creds-from-loadtable branch 6 times, most recently from 7848acb to 0c21c44 Compare March 28, 2025 14:56
@nastra nastra marked this pull request as ready for review March 28, 2025 14:57
@nastra nastra requested a review from amogh-jahagirdar March 28, 2025 14:57
@nastra nastra force-pushed the creds-from-loadtable branch 4 times, most recently from 2f64867 to 0ca4c6b Compare March 31, 2025 14:43
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

Thanks @nastra! Passing through the credentials to the fileIO implementations looks great. Just had a comment for adding a JavaDoc on the new mixin interface since it is public.

@nastra nastra force-pushed the creds-from-loadtable branch from 0ca4c6b to 7d75581 Compare April 2, 2025 06:11
@nastra
Copy link
Contributor Author

nastra commented Apr 2, 2025

thanks for the reviews @amogh-jahagirdar and @danielcweeks

@nastra nastra merged commit 817dc35 into apache:main Apr 2, 2025
42 checks passed
@nastra nastra deleted the creds-from-loadtable branch April 2, 2025 09:55
}

@Override
public void setCredentials(List<StorageCredential> credentials) {
Copy link

Choose a reason for hiding this comment

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

From what I observe, the this.storageCredentials can be wrapped in ImmutableList.copyOf. This way, credentials() doesn't have to create a copy every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will not work with Kryo ser/de because Kryo will try to add elements back after deserialization into the list and then fail

Copy link

Choose a reason for hiding this comment

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

I think the getter can be written this way (with additional field called copy):
first time getter assigns ImmutableList.copyOf() to copy field

subsequently copy field is returned directly until setter is called

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants