kbendick commented on a change in pull request #3801:
URL: https://github.com/apache/iceberg/pull/3801#discussion_r774852961
##########
File path: core/src/main/java/org/apache/iceberg/CachingCatalog.java
##########
@@ -86,16 +85,15 @@ protected CachingCatalog(Catalog catalog, boolean
caseSensitive, long expiration
}
/**
- * CacheWriter class for removing metadata tables when their associated data
table is expired
+ * RemovalListener class for removing metadata tables when their associated
data table is expired
* via cache expiration.
+ *
+ * @see com.github.benmanes.caffeine.cache.RemovalListener
*/
- class MetadataTableInvalidatingCacheWriter implements
CacheWriter<TableIdentifier, Table> {
+ class MetadataTableInvalidatingRemovalListener implements
RemovalListener<TableIdentifier, Table> {
@Override
- public void write(TableIdentifier tableIdentifier, Table table) {
- }
-
- @Override
- public void delete(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
+ public void onRemoval(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
+ identLoggingRemovalListener.onRemoval(tableIdentifier, table, cause);
Review comment:
Nit: Instead of relying on `identLoggingRemovalListener#onRemoval`, as
we're already inside of an `onRemoval` function, would it make sense to cut out
the mental overhead and just add the log statement here directly? The double
`onRemoval` was odd to me on first glance, and is added overhead for the reader.
EDIT: As mentioned elsewhere, `identLoggingRemovalLIstener` is no longer
needed (we only added it to log about cache expiration). Realistically, logging
has additional overhead and caffeine logs itself as well. How helpful to
debugging the situation for you was this specific log? If the log message
doesn't seem _critical_, I think the class should look as follows and avoid the
extra logging that was added when the `MetadataTableInvalidatingCacheWriter`
was introduced:
```
/**
* RemovalListener class for removing metadata tables when their
associated data table is expired
* via cache expiration.
*/
class MetadataTableInvalidatingRemovalListener implements
RemovalListener<TableIdentifier, Table> {
@Override
public void onRemoval(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
if (RemovalCause.EXPIRED.equals(cause) &&
!MetadataTableUtils.hasMetadataTableName(tableIdentifier)) {
tableCache.invalidateAll(metadataTableIdentifiers(tableIdentifier));
}
}
}
```
##########
File path: core/src/main/java/org/apache/iceberg/CachingCatalog.java
##########
@@ -86,16 +85,15 @@ protected CachingCatalog(Catalog catalog, boolean
caseSensitive, long expiration
}
/**
- * CacheWriter class for removing metadata tables when their associated data
table is expired
+ * RemovalListener class for removing metadata tables when their associated
data table is expired
* via cache expiration.
+ *
+ * @see com.github.benmanes.caffeine.cache.RemovalListener
*/
- class MetadataTableInvalidatingCacheWriter implements
CacheWriter<TableIdentifier, Table> {
+ class MetadataTableInvalidatingRemovalListener implements
RemovalListener<TableIdentifier, Table> {
@Override
- public void write(TableIdentifier tableIdentifier, Table table) {
- }
-
- @Override
- public void delete(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
+ public void onRemoval(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
+ identLoggingRemovalListener.onRemoval(tableIdentifier, table, cause);
Review comment:
Nit: Instead of relying on `identLoggingRemovalListener#onRemoval`, as
we're already inside of an `onRemoval` function, would it make sense to cut out
the mental overhead and just add the log statement here directly? The double
`onRemoval` was odd to me on first glance, and is added overhead for the reader.
EDIT: As mentioned elsewhere, `identLoggingRemovalListener` is no longer
needed (we only added it to log about cache expiration). Realistically, logging
has additional overhead and caffeine logs itself as well. How helpful to
debugging the situation for you was this specific log? If the log message
doesn't seem _critical_, I think the class should look as follows and avoid the
extra logging that was added when the `MetadataTableInvalidatingCacheWriter`
was introduced:
```
/**
* RemovalListener class for removing metadata tables when their
associated data table is expired
* via cache expiration.
*/
class MetadataTableInvalidatingRemovalListener implements
RemovalListener<TableIdentifier, Table> {
@Override
public void onRemoval(TableIdentifier tableIdentifier, Table table,
RemovalCause cause) {
if (RemovalCause.EXPIRED.equals(cause) &&
!MetadataTableUtils.hasMetadataTableName(tableIdentifier)) {
tableCache.invalidateAll(metadataTableIdentifiers(tableIdentifier));
}
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
