You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "andrew4699 (via GitHub)" <gi...@apache.org> on 2024/11/19 19:21:19 UTC
[PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
andrew4699 opened a new pull request, #458:
URL: https://github.com/apache/polaris/pull/458
# Description
Adds more logs. This is useful for determining the types of workloads happening on Polaris.
## Type of change
- [X] New feature (non-breaking change which adds functionality)
# How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
- [X] Added log assertions
# Checklist:
- [X] I have performed a self-review of my code
- [X] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [X] My changes generate no new warnings
- [ ] If adding new functionality, I have discussed my implementation with the community using the linked GitHub 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log the full TableMetadata [polaris]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #458:
URL: https://github.com/apache/polaris/pull/458#issuecomment-2494130544
> > @RussellSpitzer @snazy Thank you for the feedback. I'm new to the Iceberg space and appreciate the context you have on these objects. My hope is to make it easier to see the "unstructured" data that gets self-reported by the query engines. Would you feel more comfortable if this was scoped down to printing the last 5 Snapshot ID & summaries?
>
> What's the use case for logging it at all, that cannot be done from an Iceberg client or `curl`?
I have no problem with the functionality but I think it probably should be part of an eventing api. We may want to keep the complete history of the table somewhere (possibly not in the Catalog itself)
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log the full TableMetadata [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy closed pull request #458: Log the full TableMetadata
URL: https://github.com/apache/polaris/pull/458
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #458:
URL: https://github.com/apache/polaris/pull/458#discussion_r1849302135
##########
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##########
@@ -742,4 +750,85 @@ public void testRefreshToken() throws IOException {
assertThat(sessionSpy.credential()).isNotEqualTo(userToken);
}
}
+
+ @Test
+ public void testReportMetricsLogsRequest(TestInfo testInfo) throws IOException {
+ ch.qos.logback.classic.Logger logger =
+ (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(IcebergCatalogAdapter.class);
+ ListAppender<ILoggingEvent> listAppender = new ListAppender<>();
+ listAppender.start();
+ logger.addAppender(listAppender);
+
+ String catalogName = testInfo.getTestMethod().get().getName() + "External";
+ createCatalog(
+ catalogName,
+ Catalog.TypeEnum.EXTERNAL,
+ PRINCIPAL_ROLE_NAME,
+ FileStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.FILE)
+ .setAllowedLocations(List.of("file://" + testDir.toFile().getAbsolutePath()))
+ .build(),
+ "file://" + testDir.toFile().getAbsolutePath());
+ try (RESTSessionCatalog sessionCatalog = newSessionCatalog(catalogName);
+ HadoopFileIO fileIo = new HadoopFileIO(new Configuration()); ) {
+ SessionCatalog.SessionContext sessionContext = SessionCatalog.SessionContext.createEmpty();
+ Namespace ns = Namespace.of("db1");
+ sessionCatalog.createNamespace(sessionContext, ns);
+ TableIdentifier tableIdentifier = TableIdentifier.of(ns, "the_table");
+ String location =
+ "file://"
+ + testDir.toFile().getAbsolutePath()
+ + "/"
+ + testInfo.getTestMethod().get().getName();
+ String metadataLocation = location + "/metadata/000001-494949494949494949.metadata.json";
+
+ TableMetadata tableMetadata =
+ TableMetadata.buildFromEmpty()
+ .setLocation(location)
+ .assignUUID()
+ .addPartitionSpec(PartitionSpec.unpartitioned())
+ .addSortOrder(SortOrder.unsorted())
+ .addSchema(
+ new Schema(Types.NestedField.of(1, false, "col1", Types.StringType.get())), 1)
+ .build();
+ TableMetadataParser.write(tableMetadata, fileIo.newOutputFile(metadataLocation));
+
+ sessionCatalog.registerTable(sessionContext, tableIdentifier, metadataLocation);
+ Table table = sessionCatalog.loadTable(sessionContext, tableIdentifier);
+
+ int schemaId =
+ 82193782; // Keep this unique enough that the number wouldn't show up in the logs by
+ // coincedence
Review Comment:
Updated it to check a unique string instead which is slightly better.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on PR #458:
URL: https://github.com/apache/polaris/pull/458#issuecomment-2489344682
I removed the ReportMetricsRequest logging as it seems like we already log it as a tag with `Invoking CatalogApi with params`.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log the full TableMetadata [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #458:
URL: https://github.com/apache/polaris/pull/458#issuecomment-2531314457
Going to close this PR as there has been no activity on this PR, but most importantly because logging the whole table-metadata is very problematic for two reasons:
1. the shear size of that object (many kB up to MBs)
2. the fact that it contains potentially sensitive user data
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log the full TableMetadata [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #458:
URL: https://github.com/apache/polaris/pull/458#issuecomment-2493568311
> @RussellSpitzer @snazy Thank you for the feedback. I'm new to the Iceberg space and appreciate the context you have on these objects. My hope is to make it easier to see the "unstructured" data that gets self-reported by the query engines. Would you feel more comfortable if this was scoped down to printing the last 5 Snapshot ID & summaries?
What's the use case for logging it at all, that cannot be done from an Iceberg client or `curl`?
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #458:
URL: https://github.com/apache/polaris/pull/458#discussion_r1850841806
##########
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1254,6 +1255,9 @@ public void doRefresh() {
public void doCommit(TableMetadata base, TableMetadata metadata) {
LOGGER.debug(
"doCommit for table {} with base {}, metadata {}", tableIdentifier, base, metadata);
+ LOGGER.info(
+ "doCommit full new metadata: {}",
+ PolarisObjectMapperUtil.serialize(getCurrentPolarisContext(), metadata));
Review Comment:
Oh that's nice! I updated it to DEBUG and made it use the supplier syntax.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log the full TableMetadata [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on PR #458:
URL: https://github.com/apache/polaris/pull/458#issuecomment-2494384537
> What's the use case for logging it at all, that cannot be done from an Iceberg client or curl?
This is intended to be server-side so Polaris knows more about its callers.
> I have no problem with the functionality but I think it probably should be part of an eventing api. We may want to keep the complete history of the table somewhere (possibly not in the Catalog itself)
Yes I think that would be a useful API and [this change](https://github.com/apache/polaris/pull/433) could help the project move in that direction. In some sense these 2 changes can make the value proposition clearer by first providing low-friction access to this unstructured, self-reported data.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log the full TableMetadata [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on PR #458:
URL: https://github.com/apache/polaris/pull/458#issuecomment-2491916661
@RussellSpitzer @snazy Thank you for the feedback. I'm new to the Iceberg space and appreciate the context you have on these objects. My hope is to make it easier to see the "unstructured" data that gets self-reported by the query engines. Would you feel more comfortable if this was scoped down to printing the last 10 Snapshot ID & summaries?
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #458:
URL: https://github.com/apache/polaris/pull/458#discussion_r1849317616
##########
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1254,6 +1255,9 @@ public void doRefresh() {
public void doCommit(TableMetadata base, TableMetadata metadata) {
LOGGER.debug(
"doCommit for table {} with base {}, metadata {}", tableIdentifier, base, metadata);
+ LOGGER.info(
+ "doCommit full new metadata: {}",
+ PolarisObjectMapperUtil.serialize(getCurrentPolarisContext(), metadata));
Review Comment:
> Move to DEBUG (still performs serialization, does nothing for people who run in INFO+ which should be common)
Good callout. Man do I miss scala.
In this case, perhaps we can do something like:
```java
LOGGER.info(
"doCommit full new metadata: {}",
() -> PolarisObjectMapperUtil.serialize(getCurrentPolarisContext(), metadata));
```
SLF4J 2.0 [supports](https://slf4j.org/manual.html#fluent) this, and in general it might be a useful wrapper for us.
Having said all of that, I'm less worried about the serde overhead, since we do so much metadata/property serde anyway, and more about emitting a huge blob into the logs. Particularly the INFO logs which should stay high-signal.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log the full TableMetadata [polaris]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #458:
URL: https://github.com/apache/polaris/pull/458#issuecomment-2491586739
I would consider this probably a "trace" level event. I agree with @snazy's comments here. I would hesitate to log these whole entities as well for security reasons.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #458:
URL: https://github.com/apache/polaris/pull/458#discussion_r1849018695
##########
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1254,6 +1255,9 @@ public void doRefresh() {
public void doCommit(TableMetadata base, TableMetadata metadata) {
LOGGER.debug(
"doCommit for table {} with base {}, metadata {}", tableIdentifier, base, metadata);
+ LOGGER.info(
+ "doCommit full new metadata: {}",
+ PolarisObjectMapperUtil.serialize(getCurrentPolarisContext(), metadata));
Review Comment:
Yea I'm both worried about the performance hit of serializing such a large object and the amount of data logged. Some possible options:
- Move to DEBUG (still performs serialization, does nothing for people who run in INFO+ which should be common)
- Gate the entire thing behind a featureConfiguration
- Set a limit on the log output length in featureConfiguration (still performs serialization)
What are your thoughts? I'm leaning toward the 2nd one.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #458:
URL: https://github.com/apache/polaris/pull/458#discussion_r1848945266
##########
polaris-service/src/test/java/org/apache/polaris/service/PolarisApplicationIntegrationTest.java:
##########
@@ -742,4 +750,85 @@ public void testRefreshToken() throws IOException {
assertThat(sessionSpy.credential()).isNotEqualTo(userToken);
}
}
+
+ @Test
+ public void testReportMetricsLogsRequest(TestInfo testInfo) throws IOException {
+ ch.qos.logback.classic.Logger logger =
+ (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(IcebergCatalogAdapter.class);
+ ListAppender<ILoggingEvent> listAppender = new ListAppender<>();
+ listAppender.start();
+ logger.addAppender(listAppender);
+
+ String catalogName = testInfo.getTestMethod().get().getName() + "External";
+ createCatalog(
+ catalogName,
+ Catalog.TypeEnum.EXTERNAL,
+ PRINCIPAL_ROLE_NAME,
+ FileStorageConfigInfo.builder(StorageConfigInfo.StorageTypeEnum.FILE)
+ .setAllowedLocations(List.of("file://" + testDir.toFile().getAbsolutePath()))
+ .build(),
+ "file://" + testDir.toFile().getAbsolutePath());
+ try (RESTSessionCatalog sessionCatalog = newSessionCatalog(catalogName);
+ HadoopFileIO fileIo = new HadoopFileIO(new Configuration()); ) {
+ SessionCatalog.SessionContext sessionContext = SessionCatalog.SessionContext.createEmpty();
+ Namespace ns = Namespace.of("db1");
+ sessionCatalog.createNamespace(sessionContext, ns);
+ TableIdentifier tableIdentifier = TableIdentifier.of(ns, "the_table");
+ String location =
+ "file://"
+ + testDir.toFile().getAbsolutePath()
+ + "/"
+ + testInfo.getTestMethod().get().getName();
+ String metadataLocation = location + "/metadata/000001-494949494949494949.metadata.json";
+
+ TableMetadata tableMetadata =
+ TableMetadata.buildFromEmpty()
+ .setLocation(location)
+ .assignUUID()
+ .addPartitionSpec(PartitionSpec.unpartitioned())
+ .addSortOrder(SortOrder.unsorted())
+ .addSchema(
+ new Schema(Types.NestedField.of(1, false, "col1", Types.StringType.get())), 1)
+ .build();
+ TableMetadataParser.write(tableMetadata, fileIo.newOutputFile(metadataLocation));
+
+ sessionCatalog.registerTable(sessionContext, tableIdentifier, metadataLocation);
+ Table table = sessionCatalog.loadTable(sessionContext, tableIdentifier);
+
+ int schemaId =
+ 82193782; // Keep this unique enough that the number wouldn't show up in the logs by
+ // coincedence
Review Comment:
This seems liable to flake... at the very least, `1782193782` is in 2 years :)
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Log more things: TableMetadata and ReportMetricsRequest [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #458:
URL: https://github.com/apache/polaris/pull/458#discussion_r1848947076
##########
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1254,6 +1255,9 @@ public void doRefresh() {
public void doCommit(TableMetadata base, TableMetadata metadata) {
LOGGER.debug(
"doCommit for table {} with base {}, metadata {}", tableIdentifier, base, metadata);
+ LOGGER.info(
+ "doCommit full new metadata: {}",
+ PolarisObjectMapperUtil.serialize(getCurrentPolarisContext(), metadata));
Review Comment:
This can be pretty big to log into `INFO`
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org