You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wangyinsheng (via GitHub)" <gi...@apache.org> on 2023/03/19 11:56:17 UTC
[GitHub] [iceberg] wangyinsheng opened a new pull request, #7144: Core: Add metrics reporter for serializable table
wangyinsheng opened a new pull request, #7144:
URL: https://github.com/apache/iceberg/pull/7144
The metrics reporter will be lost when a table be sent to other nodes in a cluster through serializable table, This mr rebuild the metrics reporter when the table is deserialized
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1519700405
please take a look @nastra @rdblue @aokolnychyi @danielcweeks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1504987582
@wangyinsheng to fix CI failures I think you need to add the below code to `TransactionTable`
```
@Override
public MetricsReporter metricsReporter() {
return BaseTransaction.this.reporter;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add metrics reporter for serializable table [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7144: Core: Add metrics reporter for serializable table
URL: https://github.com/apache/iceberg/pull/7144
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1491139791
PTAL @aokolnychyi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1486057453
PTAL @nastra
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1164303852
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -83,6 +87,10 @@ protected SerializableTable(Table table) {
this.encryption = table.encryption();
this.locationProvider = table.locationProvider();
this.refs = SerializableMap.copyOf(table.refs());
+ this.metricsReporterProperties =
+ table instanceof BaseTable
+ ? SerializableMap.copyOf(table.metricsReporter().properties())
+ : null;
Review Comment:
we should rather do https://github.com/apache/iceberg/pull/7144#issuecomment-1504987582 than setting this to null here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1147021881
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -247,6 +252,18 @@ public Map<String, SnapshotRef> refs() {
return refs;
}
+ @Override
+ public MetricsReporter metricsReporter() {
Review Comment:
PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1519866237
@wangyinsheng as mentioned previously, we still would like to make the `MetricsReporter` `Serializable`. For things that aren't `Serializable` in a metrics reporter, we would make them `transient` and then lazily initialize them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1165347722
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -83,6 +87,10 @@ protected SerializableTable(Table table) {
this.encryption = table.encryption();
this.locationProvider = table.locationProvider();
this.refs = SerializableMap.copyOf(table.refs());
+ this.metricsReporterProperties =
+ table instanceof BaseTable
+ ? SerializableMap.copyOf(table.metricsReporter().properties())
+ : null;
Review Comment:
Sorry, I missed the comment when I try to fix the CI, I wll change it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1149339713
##########
core/src/test/java/org/apache/iceberg/TestTables.java:
##########
@@ -264,8 +265,17 @@ public void commit(TableMetadata base, TableMetadata updatedMetadata) {
throw new CommitFailedException("Injected failure");
}
Integer version = VERSIONS.get(tableName);
+ String metadataLocation =
Review Comment:
I still think those changes are not necessary. The test passed fine for me without having those changes
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -247,6 +252,18 @@ public Map<String, SnapshotRef> refs() {
return refs;
}
+ @Override
+ public MetricsReporter metricsReporter() {
+ if (lazyMetricsReporter == null) {
+ synchronized (this) {
+ if (lazyMetricsReporter == null) {
+ lazyMetricsReporter = CatalogUtil.loadMetricsReporter(this.metricsReporterProperties);
Review Comment:
sorry my bad, you're right. it should be the catalog properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1148278814
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -247,6 +252,18 @@ public Map<String, SnapshotRef> refs() {
return refs;
}
+ @Override
+ public MetricsReporter metricsReporter() {
+ if (lazyMetricsReporter == null) {
+ synchronized (this) {
+ if (lazyMetricsReporter == null) {
+ lazyMetricsReporter = CatalogUtil.loadMetricsReporter(this.metricsReporterProperties);
Review Comment:
metrics reporter is configured by catalog properties, not table properties. I add the interface method `MetricsReporter.properties()` to return the properties which used to load and initialize this `MetricsReporter`, therefor the `SerializableTable` can use this properties to load and initialize `MetricsReporter` agagin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1147617396
##########
core/src/test/java/org/apache/iceberg/TestSerializableTable.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TestHelpers.assertSerializedMetricsReporter;
+import static org.apache.iceberg.TestHelpers.roundTripSerialize;
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.io.IOException;
+import java.util.Map;
+import org.apache.iceberg.metrics.MetricsReporter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
Review Comment:
PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1141989528
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -247,6 +252,18 @@ public Map<String, SnapshotRef> refs() {
return refs;
}
+ @Override
+ public MetricsReporter metricsReporter() {
Review Comment:
can we add a test that makes sure the metrics reporter is preserved with the right properties?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1578128048
> @nastra, where are we with the fix for serialization here?
@rdblue I haven't heard back from @wangyinsheng after [my comment above ](https://github.com/apache/iceberg/pull/7144#issuecomment-1519866237), so I've opened https://github.com/apache/iceberg/pull/7370 a while ago for initial experimentation and then updated it to fix the serialization issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1506827984
@wangyinsheng I talked to @rdblue yesterday and we concluded that we don't want to carry over the catalog properties just for the metrics reporter. The better approach would be to make `MetricsReporter` extend `Serializable`. I opened https://github.com/wangyinsheng/iceberg/pull/1 against your branch to do that. Feel free to squash it into your changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1486964683
> I did take another look and I don't think we need the changes in `TestTables`, because the tests will pass without those.
you're right, it not necessary. I used to compare the metadata between origin table and serializable table, then it will be needed. I have removed the changes, PTAL @nastra
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1148279570
##########
core/src/test/java/org/apache/iceberg/TestSerializableTable.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TestHelpers.roundTripSerialize;
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Map;
+import org.apache.iceberg.metrics.MetricsReporter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+public class TestSerializableTable {
+
+ private static final Schema SCHEMA =
+ new Schema(
+ required(1, "id", Types.LongType.get()),
+ optional(2, "data", Types.StringType.get()),
+ required(3, "date", Types.StringType.get()),
+ optional(4, "double", Types.DoubleType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("date").build();
+
+ private static final SortOrder SORT_ORDER = SortOrder.builderFor(SCHEMA).asc("id").build();
+
+ @TempDir public File temp;
+
+ @AfterAll
+ public static void clean() {
+ TestTables.clearTables();
+ }
+
+ @Test
+ public void testSerializableTableWithMetricsReporter()
+ throws IOException, ClassNotFoundException {
+ Map<String, String> properties = Maps.newHashMap();
Review Comment:
metrics reporter is configured by catalog properties, not table properties. Here this `properties` is taken as properties of catalog
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1148277457
##########
core/src/test/java/org/apache/iceberg/TestSerializableTable.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TestHelpers.roundTripSerialize;
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Map;
+import org.apache.iceberg.metrics.MetricsReporter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+public class TestSerializableTable {
+
+ private static final Schema SCHEMA =
+ new Schema(
+ required(1, "id", Types.LongType.get()),
+ optional(2, "data", Types.StringType.get()),
+ required(3, "date", Types.StringType.get()),
+ optional(4, "double", Types.DoubleType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("date").build();
+
+ private static final SortOrder SORT_ORDER = SortOrder.builderFor(SCHEMA).asc("id").build();
+
+ @TempDir public File temp;
+
+ @AfterAll
+ public static void clean() {
+ TestTables.clearTables();
+ }
+
+ @Test
+ public void testSerializableTableWithMetricsReporter()
+ throws IOException, ClassNotFoundException {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(
+ CatalogProperties.METRICS_REPORTER_IMPL, TestHelpers.TestMetricsReporter.class.getName());
+ MetricsReporter reporter = CatalogUtil.loadMetricsReporter(properties);
+ Table table = TestTables.create(temp, "tbl_A", SCHEMA, SPEC, SORT_ORDER, 2, reporter);
Review Comment:
`reporter.initialize(reporterProperties)` is called in the method `CatalogUtil.loadMetricsReporter(properties)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1495715151
@nastra Could you please approval the CI ? And is there someone else can review this pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1147666190
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -247,6 +252,18 @@ public Map<String, SnapshotRef> refs() {
return refs;
}
+ @Override
+ public MetricsReporter metricsReporter() {
+ if (lazyMetricsReporter == null) {
+ synchronized (this) {
+ if (lazyMetricsReporter == null) {
+ lazyMetricsReporter = CatalogUtil.loadMetricsReporter(this.metricsReporterProperties);
Review Comment:
this is actually wrong and this should pass `properties` so that we have `CatalogUtil.loadMetricsReporter(this.properties)`
To finish the initialization of the metrics reporter after it was loaded, you'd have to do `lazyMetricsReporter.initialize(this.metricsReporterProperties)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add metrics reporter for serializable table [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-2313786049
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add metrics reporter for serializable table [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-2330357123
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1494354719
could you please take a look this pr, it has last for weeks @aokolnychyi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1165347722
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -83,6 +87,10 @@ protected SerializableTable(Table table) {
this.encryption = table.encryption();
this.locationProvider = table.locationProvider();
this.refs = SerializableMap.copyOf(table.refs());
+ this.metricsReporterProperties =
+ table instanceof BaseTable
+ ? SerializableMap.copyOf(table.metricsReporter().properties())
+ : null;
Review Comment:
Sorry, I missed the comment when I try to fix the CI failures, I wll change it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1575669356
@nastra, where are we with the fix for serialization here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1586785170
> > @nastra, where are we with the fix for serialization here?
>
> @rdblue I haven't heard back from @wangyinsheng after [my comment above ](https://github.com/apache/iceberg/pull/7144#issuecomment-1519866237), so I've opened #7370 a while ago for initial experimentation and then updated it to fix the serialization issue.
@nastra @rdblue Sorry，I am working on other things for a while. But I am still keep my opinion, I think the `SerializableTable` is the best place to handle the `lazily initialization` stuff for `MetricsReporter`, we should not just make `MetricsReporter` serializable, which will throw the `lazily initialization` stuff to any implemention class of `MetricsReporter`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1501549774
@aokolnychyi @danielcweeks could you take a look please ? thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1149337001
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -247,6 +252,18 @@ public Map<String, SnapshotRef> refs() {
return refs;
}
+ @Override
+ public MetricsReporter metricsReporter() {
+ if (lazyMetricsReporter == null) {
+ synchronized (this) {
+ if (lazyMetricsReporter == null) {
+ lazyMetricsReporter = CatalogUtil.loadMetricsReporter(this.metricsReporterProperties);
Review Comment:
sorry my bad, you're right. it should be the catalog properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1507798782
@nastra I am confused why should't carry over the catalog properties for metrics reporter? We already support to config metrics report through catalog properites. And metrics reporter may contains non-serializable object, `KafakProducer` for example. We need properties to recreate non-serializable object after table been deserialized
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1517154692
please take a look @nastra @rdblue @aokolnychyi @danielcweeks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1147175520
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -137,6 +142,18 @@ public static void assertSerializedAndLoadedMetadata(Table expected, Table actua
Assert.assertEquals("History must match", expected.history(), actual.history());
}
+ public static void assertSerializedMetricsReporter(
Review Comment:
no need to add this here, this can be just part of the test method itself
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -137,6 +142,18 @@ public static void assertSerializedAndLoadedMetadata(Table expected, Table actua
Assert.assertEquals("History must match", expected.history(), actual.history());
}
+ public static void assertSerializedMetricsReporter(
+ MetricsReporter expected, MetricsReporter actual) {
+ Assert.assertNotNull("metrics reporter from serializableTable should not be null", actual);
+ Assert.assertTrue(
+ "metrics reporter from serializableTable should be instance of TestMetricsReporter",
+ actual instanceof TestMetricsReporter);
+ Assert.assertEquals(
+ "metrics reporter from serializableTable should equals the one from origin table",
+ expected,
+ actual);
Review Comment:
rather than testing actual equality on the objects, it should be enough to make sure that the properties are the same, hence the `TestMetricsReporter` wouldn't need to implement hasCode/equals
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -137,6 +142,18 @@ public static void assertSerializedAndLoadedMetadata(Table expected, Table actua
Assert.assertEquals("History must match", expected.history(), actual.history());
}
+ public static void assertSerializedMetricsReporter(
+ MetricsReporter expected, MetricsReporter actual) {
+ Assert.assertNotNull("metrics reporter from serializableTable should not be null", actual);
+ Assert.assertTrue(
Review Comment:
this can be simplified to `Assertions.assertThat(actual).isNotNull().isInstanceOf(...)` and for the properties: `Assertions.assertThat(actual.properties).isEqualTo(...)`
##########
core/src/test/java/org/apache/iceberg/TestTables.java:
##########
@@ -264,8 +265,17 @@ public void commit(TableMetadata base, TableMetadata updatedMetadata) {
throw new CommitFailedException("Injected failure");
}
Integer version = VERSIONS.get(tableName);
+ String metadataLocation =
Review Comment:
why are these changes necessary?
##########
core/src/test/java/org/apache/iceberg/TestSerializableTable.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TestHelpers.assertSerializedMetricsReporter;
+import static org.apache.iceberg.TestHelpers.roundTripSerialize;
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.io.IOException;
+import java.util.Map;
+import org.apache.iceberg.metrics.MetricsReporter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
Review Comment:
please take a look at https://iceberg.apache.org/contribute/#testing. Basically new test classes should be JUnit5 rather than JUnit4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1147616700
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -137,6 +142,18 @@ public static void assertSerializedAndLoadedMetadata(Table expected, Table actua
Assert.assertEquals("History must match", expected.history(), actual.history());
}
+ public static void assertSerializedMetricsReporter(
Review Comment:
done
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -137,6 +142,18 @@ public static void assertSerializedAndLoadedMetadata(Table expected, Table actua
Assert.assertEquals("History must match", expected.history(), actual.history());
}
+ public static void assertSerializedMetricsReporter(
+ MetricsReporter expected, MetricsReporter actual) {
+ Assert.assertNotNull("metrics reporter from serializableTable should not be null", actual);
+ Assert.assertTrue(
+ "metrics reporter from serializableTable should be instance of TestMetricsReporter",
+ actual instanceof TestMetricsReporter);
+ Assert.assertEquals(
+ "metrics reporter from serializableTable should equals the one from origin table",
+ expected,
+ actual);
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1148277328
##########
core/src/test/java/org/apache/iceberg/TestSerializableTable.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TestHelpers.roundTripSerialize;
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Map;
+import org.apache.iceberg.metrics.MetricsReporter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+public class TestSerializableTable {
+
+ private static final Schema SCHEMA =
+ new Schema(
+ required(1, "id", Types.LongType.get()),
+ optional(2, "data", Types.StringType.get()),
+ required(3, "date", Types.StringType.get()),
+ optional(4, "double", Types.DoubleType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("date").build();
+
+ private static final SortOrder SORT_ORDER = SortOrder.builderFor(SCHEMA).asc("id").build();
+
+ @TempDir public File temp;
+
+ @AfterAll
+ public static void clean() {
+ TestTables.clearTables();
+ }
+
+ @Test
+ public void testSerializableTableWithMetricsReporter()
+ throws IOException, ClassNotFoundException {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(
Review Comment:
done
##########
core/src/test/java/org/apache/iceberg/TestSerializableTable.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.apache.iceberg.TestHelpers.roundTripSerialize;
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Map;
+import org.apache.iceberg.metrics.MetricsReporter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+public class TestSerializableTable {
+
+ private static final Schema SCHEMA =
+ new Schema(
+ required(1, "id", Types.LongType.get()),
+ optional(2, "data", Types.StringType.get()),
+ required(3, "date", Types.StringType.get()),
+ optional(4, "double", Types.DoubleType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("date").build();
+
+ private static final SortOrder SORT_ORDER = SortOrder.builderFor(SCHEMA).asc("id").build();
+
+ @TempDir public File temp;
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1148277358
##########
api/src/main/java/org/apache/iceberg/Table.java:
##########
@@ -346,4 +347,14 @@ default Snapshot snapshot(String name) {
return null;
}
+
+ /**
+ * Returns the metrics reporter for this table.
+ *
+ * @return the metrics reporter for this table.
+ */
+ default MetricsReporter metricsReporter() {
+ throw new UnsupportedOperationException(
+ "metricsReporter is not supported by " + getClass().getName());
Review Comment:
done
##########
api/src/test/java/org/apache/iceberg/TestHelpers.java:
##########
@@ -559,4 +561,21 @@ public List<Long> splitOffsets() {
return null;
}
}
+
+ public static class TestMetricsReporter implements MetricsReporter {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1489527262
PTAL @aokolnychyi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7144: Core: Add metrics reporter for serializable table
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#discussion_r1141989528
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -247,6 +252,18 @@ public Map<String, SnapshotRef> refs() {
return refs;
}
+ @Override
+ public MetricsReporter metricsReporter() {
Review Comment:
can we add a test that makes sure the metrics reporter is preserved?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] wangyinsheng commented on pull request #7144: Core: Add metrics reporter for serializable table
Posted by "wangyinsheng (via GitHub)" <gi...@apache.org>.
wangyinsheng commented on PR #7144:
URL: https://github.com/apache/iceberg/pull/7144#issuecomment-1475855068
PTAL @nastra
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org