Skip to content

Core: Make metrics reporter serializable #7370

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.

Sign up for GitHub

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

Jump to bottom
Closed
nastra wants to merge 1 commit into from
Closed

Core: Make metrics reporter serializable #7370

nastra wants to merge 1 commit into from

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Apr 18, 2023 •
edited
Loading

No description provided.

@nastra nastra force-pushed the serializable-metrics-reporter branch 4 times, most recently from fb3fc4a to 5830956 Compare April 20, 2023 08:22
@nastra nastra marked this pull request as draft April 20, 2023 08:23
@nastra nastra force-pushed the serializable-metrics-reporter branch from 5830956 to 97aeb00 Compare April 20, 2023 08:27
@nastra nastra mentioned this pull request Apr 24, 2023
Merged
@nastra nastra force-pushed the serializable-metrics-reporter branch 2 times, most recently from 7ccb9d8 to 8276986 Compare April 25, 2023 18:00
rdblue
rdblue reviewed Apr 25, 2023
View reviewed changes
core/src/main/java/org/apache/iceberg/io/ResolvingFileIO.java
String impl = implFromLocation(location);
FileIO io = ioInstances.get(impl);
if (io != null) {
if (io instanceof HadoopFileIO && ((HadoopFileIO) io).conf() == null) {
Copy link
Contributor

@rdblue rdblue Apr 25, 2023

Choose a reason for hiding this comment

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

Why does this need to change IO classes?

Copy link
Contributor Author

@nastra nastra Apr 26, 2023

Choose a reason for hiding this comment

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

Sorry, forgot to mention why I added this. It appears that the underlying conf of HadoopFileIO wasn't initialized when calling StaticTableOperations.current():

hadoop_io_conf_missing

@nastra nastra force-pushed the serializable-metrics-reporter branch 4 times, most recently from 8e34d90 to 35c58f6 Compare April 26, 2023 08:42
nastra
nastra commented Apr 26, 2023
View reviewed changes
core/src/main/java/org/apache/iceberg/util/SerializableSet.java
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;

public class SerializableSet<E> implements Set<E>, Serializable {
Copy link
Contributor Author

@nastra nastra Apr 26, 2023

Choose a reason for hiding this comment

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

the idea here is aligned with SerializableMap. We need SerializableSet in the CompositeMetricsReporter to be able to serialize it

@nastra nastra marked this pull request as ready for review April 26, 2023 08:58
@nastra nastra force-pushed the serializable-metrics-reporter branch 2 times, most recently from 15e0566 to 5c8199e Compare April 26, 2023 09:44
nastra
nastra commented Apr 26, 2023
View reviewed changes
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
} else {
assertThat(headers).containsAllEntriesOf(contextHeaders);
adaptor =
Mockito.spy(
Copy link
Contributor Author

@nastra nastra Apr 26, 2023

Choose a reason for hiding this comment

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

this is only wrapping the RESTCatalogAdapter in a Mockito.spy() call

@nastra nastra mentioned this pull request Apr 27, 2023
Closed
@nastra nastra force-pushed the serializable-metrics-reporter branch from 5c8199e to 21a2402 Compare May 13, 2023 06:48
@nastra nastra requested a review from rdblue May 13, 2023 06:49
rdblue
rdblue reviewed May 18, 2023
View reviewed changes
.palantir/revapi.yml
\ java.lang.String>, org.apache.iceberg.rest.RESTClient>===)"
new: "parameter void org.apache.iceberg.rest.RESTCatalog::<init>(===org.apache.iceberg.util.SerializableFunction<java.util.Map<java.lang.String,\
\ java.lang.String>, org.apache.iceberg.rest.RESTClient>===)"
justification: "Switching to SerializableFunction"
Copy link
Contributor

@rdblue rdblue May 18, 2023

Choose a reason for hiding this comment

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

Can we support both? I don't think we can make this change if it is a breaking one.

Copy link
Contributor Author

@nastra nastra May 19, 2023

Choose a reason for hiding this comment

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

I don't think we can support both because there's no place where we could just cast from Function to SerializableFunction

Copy link
Contributor Author

@nastra nastra Jul 10, 2023

Choose a reason for hiding this comment

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

@rdblue if we don't want to break the API with the Function -> SerializableFunction change, then the other alternative would be to make HTTPClient serializable. I've opened #8032 to do that

rdblue
rdblue reviewed May 18, 2023
View reviewed changes
core/src/main/java/org/apache/iceberg/SerializableTable.java
this.encryption = table.encryption();
this.locationProvider = table.locationProvider();
this.refs = SerializableMap.copyOf(table.refs());
this.metricsReporter = table.metricsReporter();
Copy link
Contributor

@rdblue rdblue May 18, 2023

Choose a reason for hiding this comment

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

Can we expose this through BaseTable instead of the Table interface? I'm not sure that we want this in the Table interface and it would be best to avoid it.

@nastra nastra requested a review from rdblue June 6, 2023 07:58
@nastra nastra mentioned this pull request Jun 6, 2023
Closed
@nastra nastra force-pushed the serializable-metrics-reporter branch from 21a2402 to 090604d Compare July 3, 2023 14:35
@nastra nastra mentioned this pull request Jul 10, 2023
Closed
@nastra nastra mentioned this pull request Aug 9, 2023
Merged
@rdblue
Copy link
Contributor

rdblue commented Sep 4, 2023

Closing this in favor of #8032 that doesn't require breaking changes.

@nastra nastra closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdblue rdblue Awaiting requested review from rdblue

Assignees

No one assigned

Labels

API core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nastra @rdblue