You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "henrib (via GitHub)" <gi...@apache.org> on 2025/06/20 13:27:51 UTC
[PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
henrib opened a new pull request, #5882:
URL: https://github.com/apache/hive/pull/5882
- add an event listener to invalidate cached tables impervious to source of change (direct HMS or REST);
- added configuration option for event class handler;
- lengthened default cache TTL;
### What changes were proposed in this pull request?
Adding an event listener to Metastore when hosting an Iceberg REST Catalog to invalidate tables (and namespaces) independently of the source of change.
### Why are the changes needed?
Cache handling for REST Iceberg catalog may return stale tables.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Unit tests
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, can't we reuse `cachedTable` here instead of doing another HMS call?
````
metadataLocation = cachedTable.getParameters().get(METADATA_LOCATION_PROP)
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267129465
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
1 remark: `hive-catalog` is a clone of iceberg's `hive-metastore`. So whenever we need to apply changes there - we should raise a PR for iceberg as well.
maybe instead of adding `getTableLocation` we could use `getMetadataLocation(catalog.loadTable(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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2175597392
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -35,19 +38,33 @@
* Catalog & servlet factory.
*/
public class HMSCatalogFactory {
- private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
+ private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
- private final Configuration configuration;
- private final int port;
- private final String path;
+ /**
+ * Convenience soft reference to last catalog.
+ */
+ protected static final AtomicReference<Reference<Catalog>> catalogRef = new AtomicReference<>();
Review Comment:
This is the only way I found to pass the catalog instance to the event listener. This is a partial solution (non HA); the HA version would require polling from the notifications through a thread and there would still be a propagation delay and potential stale entries in 'distributed' caches; a performance trade-off...
Shall I close this ? No point in continuing if you've already reached hard '-1'.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "saihemanth-cloudera (via GitHub)" <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2159788643
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -24,30 +24,45 @@
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
* @param <CATALOG> the catalog class
*/
public class HMSCachingCatalog<CATALOG extends Catalog & SupportsNamespaces> extends CachingCatalog implements SupportsNamespaces {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
protected final CATALOG nsCatalog;
public HMSCachingCatalog(CATALOG catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
nsCatalog = catalog;
}
public CATALOG hmsUnwrap() {
return nsCatalog;
}
+ public void invalidateTable(String dbName, String tableName) {
+ super.invalidateTable(TableIdentifier.of(dbName, tableName));
Review Comment:
I think we can call l#60 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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2256420632
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
I got your point, and I respectfully have a small challenge 😃 . I have two points about why we can avoid `protected` here. First, I suppose most Java projects recommend using as restrictive accessibility[1] as possible, and Hive's source code follows the principle[2]. We need use cases when we use generous modifiers. Second, we can use not an inheritance but a composition when we really want to add some traits to `HMSCatalogFactory`; We will lose flexibility with `protected`.
- [1] [Controlling Access to Members of a Class](https://docs.oracle.com/javase/tutorial/java/javaOO/accesscontrol.html), Effective Java
- [2] I understand we have some implementation that exceptionally abuses non-private modifiers
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267129465
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
1 remark: `hive-catalog` is a clone of iceberg's `hive-metastore`. So whenever we need to apply changes there - we should raise a PR for iceberg as well.
maybe instead of adding getTableLocation we could use `getMetadataLocation(catalog.loadTable(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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2182641070
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
Let me try to explain my intention. As you know, the location of the metadata top is stored in `metadata_location`. The most important point is `file:/opt/hive/data/warehouse/test/metadata/00000-165e08e2-9cea-47b1-9eef-45921f238fe2.metadata.json` is immutable. Whenever we update the table, the path will change.
```
| CREATE EXTERNAL TABLE `test`( |
| `id` int) |
| ROW FORMAT SERDE |
| 'org.apache.iceberg.mr.hive.HiveIcebergSerDe' |
| STORED BY |
| 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler' |
| |
| LOCATION |
| 'file:/opt/hive/data/warehouse/test' |
| TBLPROPERTIES ( |
| 'TRANSLATED_TO_EXTERNAL'='TRUE', |
| 'bucketing_version'='2', |
| 'current-schema'='{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"id","required":false,"type":"int"}]}', |
| 'external.table.purge'='TRUE', |
| 'format-version'='2', |
| 'iceberg.orc.files.only'='false', |
| 'metadata_location'='file:/opt/hive/data/warehouse/test/metadata/00000-165e08e2-9cea-47b1-9eef-45921f238fe2.metadata.json', |
```
Therefore, the following code does not include the staleness problem.
```java
Map<String, org.apache.iceberg.Table> cache = new HashMap(); // We should use Guava's cache or something
cache.put("file:/opt/hive/data/warehouse/test/metadata/00000-165e08e2-9cea-47b1-9eef-45921f238fe2.metadata.json", icebergTable);
```
On `loadTable`, we will use the cache in the following way.
```java
org.apache.hadoop.hive.metastore.api.Table hmsTable = rdbms.getTable(tableName);
String metadataLocation = hmsTable.getParameters().get("metadata_location"); // this might be wrong
org.apache.iceberg.Table cachedTable = cache.get(metaedataLocation);
if (cachedTable != null) {
return cachedTable;
}
// We will access S3, and then build the org.apache.iceberg.Table
```
This is suboptimal because we access the RDBMS every time. However, we can eliminate S3 latency without staleness issues. I haven't run the PoC, so I'm not sure if it really works.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2183135103
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
Actually, with the same logic and reusing the existing code, on loadTable() we can verify that if a table is cached, its location is the same as the one known in Hive DB.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-2992706104
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2255900496
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -35,24 +36,22 @@
import org.apache.iceberg.hive.HiveCatalog;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
*/
public class HMSCachingCatalog extends CachingCatalog implements SupportsNamespaces, ViewCatalog {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
private final HiveCatalog hiveCatalog;
public HMSCachingCatalog(HiveCatalog catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
Review Comment:
I could not agree more.
```
0: jdbc:hive2://localhost:10000/> create table tEsT (nAmE STRING);
...
0: jdbc:hive2://localhost:10000/> show create table test;
...
+----------------------------------------------------+
| createtab_stmt |
+----------------------------------------------------+
| CREATE EXTERNAL TABLE `test`( |
| `name` string) |
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2266022332
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
cc @okumin
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267137876
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
+ if (location == null) {
+ LOG.debug("Table {} has no location, returning cached table without location", canonicalized);
+ } else if (!location.equals(cachedTable.location())) {
+ LOG.debug("Cached table {} has a different location than the one in the catalog: {} != {}",
+ canonicalized, cachedTable.location(), location);
+ // Invalidate the cached table if the location is different
+ invalidateTable(canonicalized);
+ } else {
+ LOG.debug("Returning cached table: {}", canonicalized);
+ return cachedTable;
+ }
+ }
+ Table table = cache.get(canonicalized, catalog::loadTable);
+ if (table instanceof BaseMetadataTable) {
Review Comment:
are we only supposed to cache metadata tables?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2159892865
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -24,30 +24,45 @@
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
* @param <CATALOG> the catalog class
*/
public class HMSCachingCatalog<CATALOG extends Catalog & SupportsNamespaces> extends CachingCatalog implements SupportsNamespaces {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
protected final CATALOG nsCatalog;
public HMSCachingCatalog(CATALOG catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
nsCatalog = catalog;
}
public CATALOG hmsUnwrap() {
return nsCatalog;
}
+ public void invalidateTable(String dbName, String tableName) {
+ super.invalidateTable(TableIdentifier.of(dbName, tableName));
+ }
+
+ @Override
+ public void invalidateTable(TableIdentifier tableIdentifier) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Avoid invalidating table: {}", tableIdentifier);
+ }
Review Comment:
The idea is to try and have all actual invalidation performed through the listener; this override voids the default call on purpose.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2160365602
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
I'm new to this API. Does this work even when we set up HMS with HA with `metastore.thrift.uri.selection=RANDOM`?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2251813193
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +62,27 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ TableIdentifier canonicalIdentifier = identifier.toLowerCase();
+ Table cachedTable = tableCache.getIfPresent(canonicalIdentifier);
+ if (cachedTable != null) {
+ String location = hiveCatalog.getTableLocation(canonicalIdentifier);
+ if (location == null) {
+ LOG.debug("Table {} has no location, returning cached table without location", canonicalIdentifier);
+ } else if (!location.equals(cachedTable.location())) {
+ LOG.debug("Cached table {} has a different location than the one in the catalog: {} != {}",
+ canonicalIdentifier, cachedTable.location(), location);
+ } else {
+ LOG.debug("Returning cached table: {}", canonicalIdentifier);
+ return cachedTable;
+ }
+ // Invalidate the cached table if the location is different
+ tableCache.invalidate(cachedTable);
Review Comment:
Updated to a more robust and more efficient version.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, can't we reuse `cachedTable` here instead of doing another HMS call?
````
TableMetadata currentMetadata = ((HasTableOperations) cachedTable).operations().current();
String location = currentMetadata.metadataFileLocation();
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267129465
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
1 remark: hive-catalog is a clone of iceberg's hive-metastore. So if we really need to apply changes there - we should raise a PR for iceberg as well.
maybe instead of adding getTableLocation we could use `getMetadataLocation(catalog.loadTable(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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2182897639
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
Ok, got it; by using the metadata location as table cache key & peeking it in the RDBMS each time we loadTable, we avoid accessing a stale version of the table. We can still actively remove known stales from the cache and have some TTL/expiry to avoid keeping tables accessed once in the cache.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2182649927
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
If we cache the RDBMS entry, I think it can be on the pure HMS side. I haven't checked if it's possible or not.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267011081
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
> location objects won't ever match !!! one returns metadata file location and another just the root table location
You are absolutely right, I must missed that during debug, thanks for catching it. It could have been fixed with a call to:
` private static String getMetadataLocation(final Table table) {
if (table instanceof HasTableOperations tableOps) {
final TableOperations ops = tableOps.operations();
final TableMetadata meta;
if (ops != null && (meta = ops.current()) != null) {
return meta.metadataFileLocation();
}
}
return null;
}
`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267011081
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
> location objects won't ever match !!! one returns metadata file location and another just the root table location
You are absolutely right, I must missed that during debug, thanks for catching it. It could have been fixed with a call to:
`
private static String getMetadataLocation(final Table table) {
if (table instanceof HasTableOperations tableOps) {
final TableOperations ops = tableOps.operations();
final TableMetadata meta;
if (ops != null && (meta = ops.current()) != null) {
return meta.metadataFileLocation();
}
}
return null;
}
`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3180165389
Newer version in https://github.com/apache/hive/pull/6022 with an actual test that verifies the correct caching behavior.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, can't we reuse `cachedTable` here instead of doing another HMS call?
````
location = cachedTable.getParameters().get(METADATA_LOCATION_PROP)
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3157746030
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2259166224
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
Just for your information, I'm not confident about what our way should be. It may turn out that you may be right. Regardless of the decision, this PR looks awesome already 🏆 ; The caching feature is probably more needed than the convenience of the potential user-side extensibility. As our dev or review capacity is limited[1], I prefer to merge the important part first.
- [1] [I review many PRs in parallel](https://okumin.com/wip/), but I can work with my laptop for only 1-2 hours per day because of a medical reason
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2259567372
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
Extensibility removed from factory in last iteration.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2266026469
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
location objects won't ever match !!!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, did you even test the code ??? I don't see even a single test that validates the caching functionality
`catalog.getTableLocation(canonicalized)`
file:/var/folders/qy/whx78wjd1w72_z8_wgqfwd_c0000gn/T/RESTCatalogServer_b5e4f53d-d4c9-4b1a-8897-c9b9e82ef29e/external/newdb.db/newtable/metadata/00000-fd422e1e-bf30-4529-9baf-606662a2253a.metadata.json
`cachedTable.location()`
file:/var/folders/qy/whx78wjd1w72_z8_wgqfwd_c0000gn/T/RESTCatalogServer_b5e4f53d-d4c9-4b1a-8897-c9b9e82ef29e/external/newdb.db/newtable
one returns metadata file location and another just the root table location. I am reverting this
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2175641657
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
Wouldn't caching the file-system layer from the table layer (ie B) somehow be redundant ? The (distributed) fs and UGI already cache some info, dont they ?
I understand that caching the Table object itself comes with a propagation/staleness trade-off in HA mode but can't this be a valid one ? Any user transaction will run on a given snapshot, not necessarily the last 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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2182649927
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
If we cache the RDBMS entry, I think it can be on the pure HMS side. I haven't checked if it's possible or not.
P.S. This means, if we can cache `table_type=ICEBERG`, we can probably cache other types of tables. Iceberg REST can be just one user of the caching mechanism if we can push it into the normal HMS side. If it is impossible, probably, it could be a sign that we should not cache `table_type=ICEBERG`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2161250220
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
HMS only triggers MetaStoreEventListener for 'local' events so HA would need to disable the cache (for now). A potential solution would be to actively poll events from each HMS instance (need a dedicated thread that polls the NotificationEventRequest and follows the same invalidation logic).
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2164423493
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
As I shared it with Jira, HiveCatalog might access two types of remote resources.
- (A) Pointers from table names to the metadata location in the backend RDBMS
- (B) The content of metadata.json on Amazon S3
I'm wondering if we can update HiveCatalog to allow caching (B). The metadata.json is immutable. So, we can safely cache it.
I'd say RDMBS for Iceberg pointers is fast enough, such as 10k RPS.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3050045346
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2159889174
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -24,30 +24,45 @@
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
* @param <CATALOG> the catalog class
*/
public class HMSCachingCatalog<CATALOG extends Catalog & SupportsNamespaces> extends CachingCatalog implements SupportsNamespaces {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
protected final CATALOG nsCatalog;
public HMSCachingCatalog(CATALOG catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
nsCatalog = catalog;
}
public CATALOG hmsUnwrap() {
return nsCatalog;
}
+ public void invalidateTable(String dbName, String tableName) {
+ super.invalidateTable(TableIdentifier.of(dbName, tableName));
Review Comment:
This version of invalidateTable() is called by the listener and we do want the invalidation performed here. The override at l#60 does nothing on purpose; we do want all invalidations performed through the listener.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, can't we reuse `cachedTable` here instead of doing another HMS call?
````
lTableMetadata currentMetadata = ((HasTableOperations) cachedTable).operations().current();
String location = currentMetadata.metadataFileLocation();
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2266022332
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
cc @okumin
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "saihemanth-cloudera (via GitHub)" <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2159788765
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -24,30 +24,45 @@
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
* @param <CATALOG> the catalog class
*/
public class HMSCachingCatalog<CATALOG extends Catalog & SupportsNamespaces> extends CachingCatalog implements SupportsNamespaces {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
protected final CATALOG nsCatalog;
public HMSCachingCatalog(CATALOG catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
nsCatalog = catalog;
}
public CATALOG hmsUnwrap() {
return nsCatalog;
}
+ public void invalidateTable(String dbName, String tableName) {
+ super.invalidateTable(TableIdentifier.of(dbName, tableName));
+ }
+
+ @Override
+ public void invalidateTable(TableIdentifier tableIdentifier) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Avoid invalidating table: {}", tableIdentifier);
+ }
Review Comment:
We can then do super.invalidateTable() 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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-2993451522
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2268716822
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
> location objects won't ever match !!!
You're right. My bad. Let's add an addendum patch.
> 1 remark: hive-catalog is a clone of iceberg's hive-metastore. So whenever we need to apply changes there - we should raise a PR for iceberg as well.
I'm not finding the policy about how to manage cloned files, but it is also right anyway. I'm wondering if we can push the entire caching feature into HiveCatalog in apache/iceberg. I can try it in my spare time if no one tries it.
> @okumin, I think similar thing was discussed in https://lists.apache.org/thread/zb5ldjfxvjz4fjwll1lc9cm8rl015zx2
Thanks for identifying the related discussion. We might want to implement the finally-decided solution, probably the ETag-based one.
- https://github.com/apache/iceberg/issues/11766
- https://docs.google.com/document/d/1rnVSP_iv2I47giwfAe-Z3DYhKkKwWCVvCkC9rEvtaLA/edit?tab=t.0#heading=h.5voryi2hj0go
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2268716822
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
> location objects won't ever match !!!
You're right. My bad. Let's add an addendum patch.
> 1 remark: hive-catalog is a clone of iceberg's hive-metastore. So whenever we need to apply changes there - we should raise a PR for iceberg as well.
I'm not finding the policy about how to manage cloned files, but it is also right anyway. I'm wondering if we can push the entire caching feature into HiveCatalog in apache/iceberg. I can try it in my spare time if no one tries it. Probably off topic
> @okumin, I think similar thing was discussed in https://lists.apache.org/thread/zb5ldjfxvjz4fjwll1lc9cm8rl015zx2
Thanks for identifying the related discussion. We might want to implement the finally-decided solution, probably the ETag-based one.
- https://github.com/apache/iceberg/issues/11766
- https://docs.google.com/document/d/1rnVSP_iv2I47giwfAe-Z3DYhKkKwWCVvCkC9rEvtaLA/edit?tab=t.0#heading=h.5voryi2hj0go
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267011081
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
> location objects won't ever match !!! one returns metadata file location and another just the root table location
You are absolutely right, I must missed that during debug, thanks for catching it. It could have been fixed with a call to:
```
private static String getMetadataLocation(final Table table) {
if (table instanceof HasTableOperations tableOps) {
final TableOperations ops = tableOps.operations();
final TableMetadata meta;
if (ops != null && (meta = ops.current()) != null) {
return meta.metadataFileLocation();
}
}
return null;
}
```
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2259109607
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
Thanks. Can we separate it from this PR? The title of this PR is "Fixing cache handling for REST catalog" and your point does not belong to the original interest of this PR.
I suppose we need to clarify the three points to justify the new ticket. One, we need clear use cases to create a subclass. I don't rule out the potential use cases, but "potential" does not justify the change; Every class is likely to have a potential use case to be inherited. Two, we will need to be confident that `extends HMSCatalogFactory` is the best method to satisfy the use case. We are recently removing inheritance[1] because we failed to manage inflated complexity using inheritance. I suppose we would likely be more successful with YAGNI in this case; We can add the best abstraction, which will have real users.
I can approve and merge this PR if we can separate the problem from this PR.
- [1] https://docs.google.com/document/d/1lppPj35fBOOCjGNIouoJCsniMPpyzWx3zcE7kvu0Be4/edit?tab=t.0#heading=h.1upmmt6vhhfa
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2175569578
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -35,19 +38,33 @@
* Catalog & servlet factory.
*/
public class HMSCatalogFactory {
- private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
+ private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
- private final Configuration configuration;
- private final int port;
- private final String path;
+ /**
+ * Convenience soft reference to last catalog.
+ */
+ protected static final AtomicReference<Reference<Catalog>> catalogRef = new AtomicReference<>();
Review Comment:
remove it
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2183168553
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -35,19 +38,33 @@
* Catalog & servlet factory.
*/
public class HMSCatalogFactory {
- private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
+ private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
- private final Configuration configuration;
- private final int port;
- private final String path;
+ /**
+ * Convenience soft reference to last catalog.
+ */
+ protected static final AtomicReference<Reference<Catalog>> catalogRef = new AtomicReference<>();
Review Comment:
I've removed this and changed the logic in the latest iteration to check whether a cached table still points to the most up to date location (reusing @okumin idea).
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "saihemanth-cloudera (via GitHub)" <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2159788765
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -24,30 +24,45 @@
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.CachingCatalog;
+import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
* @param <CATALOG> the catalog class
*/
public class HMSCachingCatalog<CATALOG extends Catalog & SupportsNamespaces> extends CachingCatalog implements SupportsNamespaces {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
protected final CATALOG nsCatalog;
public HMSCachingCatalog(CATALOG catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
nsCatalog = catalog;
}
public CATALOG hmsUnwrap() {
return nsCatalog;
}
+ public void invalidateTable(String dbName, String tableName) {
+ super.invalidateTable(TableIdentifier.of(dbName, tableName));
+ }
+
+ @Override
+ public void invalidateTable(TableIdentifier tableIdentifier) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Avoid invalidating table: {}", tableIdentifier);
+ }
Review Comment:
We can then do super.invalidateTable() here? Also, why not do `nsCatalog.invalidateTable()` 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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3163374693
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2259166224
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
Just for your information, I'm not confident about what our way should be. It may turn out that you may be right. Regardless of the decision, this PR looks awesome already 🏆 ; The caching feature is probably more needed than the convenience of the potential user-side extensibility. As our dev or review capacity is limited[1], I prefer to merge the important part first.
- [1] [I review many PRs in parallel](https://okumin.com/wip/), but I can work with my laptop for only 1-2 hours per day because of a medical reason
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, can't we reuse `cachedTable` here instead of doing another HMS call?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, did you even test the code ???
`catalog.getTableLocation(canonicalized)`
file:/var/folders/qy/whx78wjd1w72_z8_wgqfwd_c0000gn/T/RESTCatalogServer_b5e4f53d-d4c9-4b1a-8897-c9b9e82ef29e/external/newdb.db/newtable/metadata/00000-fd422e1e-bf30-4529-9baf-606662a2253a.metadata.json
`cachedTable.location()`
file:/var/folders/qy/whx78wjd1w72_z8_wgqfwd_c0000gn/T/RESTCatalogServer_b5e4f53d-d4c9-4b1a-8897-c9b9e82ef29e/external/newdb.db/newtable
one returns metadata file location and another just the root table location. I am reverting this
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2267129465
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
1 remark: `hive-catalog` is a clone of iceberg's `hive-metastore`. So whenever we need to apply changes there - we should raise a PR for iceberg as well.
maybe instead of adding `getTableLocation` we could use
````
getMetadataLocation(catalog.loadTable(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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2268820357
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
+ if (location == null) {
+ LOG.debug("Table {} has no location, returning cached table without location", canonicalized);
+ } else if (!location.equals(cachedTable.location())) {
+ LOG.debug("Cached table {} has a different location than the one in the catalog: {} != {}",
+ canonicalized, cachedTable.location(), location);
+ // Invalidate the cached table if the location is different
+ invalidateTable(canonicalized);
+ } else {
+ LOG.debug("Returning cached table: {}", canonicalized);
+ return cachedTable;
+ }
+ }
+ Table table = cache.get(canonicalized, catalog::loadTable);
+ if (table instanceof BaseMetadataTable) {
Review Comment:
Line 92 does cache non-metadata table. Per the caffeine doc, cache 'get' obeys "if cached, return; otherwise create, cache and return" pattern ; line 93 and after are meant to deal with the specifics of metadata table.
What am I missing ?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265976587
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
To ensure the cached table is not stale, we check against the stored reference that the location it points to is still current. This situation (staleness) can occur if users are updating the table from HMS/Hive concurrently with through Catalog and/or HA. @okumin do you concur ?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2266091063
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
> I suppose the server-side eventual consistency requires us to design and advertise the consistency semantics carefully. I'd say the client side should do such a control.
please see https://lists.apache.org/thread/zb5ldjfxvjz4fjwll1lc9cm8rl015zx2
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin merged PR #5882:
URL: https://github.com/apache/hive/pull/5882
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2250543039
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -35,24 +36,22 @@
import org.apache.iceberg.hive.HiveCatalog;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
*/
public class HMSCachingCatalog extends CachingCatalog implements SupportsNamespaces, ViewCatalog {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
private final HiveCatalog hiveCatalog;
public HMSCachingCatalog(HiveCatalog catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
Review Comment:
I'm curious about pros vs cons of the case sensitivity here.
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +62,27 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ TableIdentifier canonicalIdentifier = identifier.toLowerCase();
+ Table cachedTable = tableCache.getIfPresent(canonicalIdentifier);
+ if (cachedTable != null) {
+ String location = hiveCatalog.getTableLocation(canonicalIdentifier);
+ if (location == null) {
+ LOG.debug("Table {} has no location, returning cached table without location", canonicalIdentifier);
+ } else if (!location.equals(cachedTable.location())) {
+ LOG.debug("Cached table {} has a different location than the one in the catalog: {} != {}",
+ canonicalIdentifier, cachedTable.location(), location);
+ } else {
+ LOG.debug("Returning cached table: {}", canonicalIdentifier);
+ return cachedTable;
+ }
+ // Invalidate the cached table if the location is different
+ tableCache.invalidate(cachedTable);
Review Comment:
I guess it's a little more robust to use `CachingCatalog#invalidateTable`. Please correct me if the current implementation is intentional.
https://github.com/apache/iceberg/blob/apache-iceberg-1.9.2/core/src/main/java/org/apache/iceberg/CachingCatalog.java#L186-L191
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog; [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2255904055
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
*/
public class HMSCatalogFactory {
- private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
+ private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
Review Comment:
```suggestion
private static final String SERVLET_ID_KEY = "metastore.in.test.iceberg.catalog.servlet.id";
```
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
What extends this class? If nothing does it, I'd like you to revert the visibility of methods and variables from `protected` to `private`.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2250759167
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -35,24 +36,22 @@
import org.apache.iceberg.hive.HiveCatalog;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Class that wraps an Iceberg Catalog to cache tables.
*/
public class HMSCachingCatalog extends CachingCatalog implements SupportsNamespaces, ViewCatalog {
+ private static final Logger LOG = LoggerFactory.getLogger(HMSCachingCatalog.class);
private final HiveCatalog hiveCatalog;
public HMSCachingCatalog(HiveCatalog catalog, long expiration) {
- super(catalog, true, expiration, Ticker.systemTicker());
+ super(catalog, false, expiration, Ticker.systemTicker());
Review Comment:
Hive is case-insensitive for metadata including table names ; not sure if catalog caller normalizes it so taking the safe choice (belt & suspender).
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2256297226
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
It is meant to allow easy extension (different versions of Iceberg REST API) and ease implementation of added features (like authorization handling); it does not harm Hive to keep this convenient way to add features with no disruption to the code line.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2256738678
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java:
##########
@@ -33,21 +33,23 @@
/**
* Catalog & servlet factory.
+ * <p>This class is derivable on purpose; the factory class name is a configuration property, this class
+ * can serve as a base for specialization.</p>
Review Comment:
I hear and welcome your challenge 😃 ; let me try and further explain why I believe this 'pattern of extensible inheritable factory' is an acceptable compromise in this context.
I actually embrace Mr Bloch rule phrased as 'favor composition over inheritance'; we are in a delineated case where we only want to allow real subtype of our factory.
If this factory class were to become composable, this would imply we can delegate to an instance and call its methods from a user defined class instance; then all its methods should become public including its constructor. And this could lead towards exposing an interface, adding even more visibility and more complexity.
If we switch back to private, no user defined class can use Hive's instance which will force anyone to duplicate (almost) all of its code to add specialization. Certainly a better alternative than the previous one at the expense of code duplication.
The protected version allows surgical precision in extension. Since the catalog cache already delegates to the Catalog instance, it is a very attractive and convenient extension point. We could even make that more obvious by exposing a cache wrapping method.
Let me know what you think.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3145225786
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3016105285
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "henrib (via GitHub)" <gi...@apache.org>.
henrib commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2161170504
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSEventListener.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * 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.rest;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.MetaStoreEventListener;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.events.AlterTableEvent;
+import org.apache.hadoop.hive.metastore.events.DropDatabaseEvent;
+import org.apache.hadoop.hive.metastore.events.DropTableEvent;
+import org.apache.hadoop.hive.metastore.events.ReloadEvent;
+import org.apache.iceberg.catalog.Catalog;
+import org.slf4j.Logger;
+
+/**
+ * IcebergEventListener is a Hive Metastore event listener that invalidates the cache
+ * of the HMSCachingCatalog when certain events occur, such as altering or dropping a table.
+ */
+public class HMSEventListener extends MetaStoreEventListener {
Review Comment:
Good question, I would hope so, let me try and seek a definite answer.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3020918534
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "okumin (via GitHub)" <gi...@apache.org>.
okumin commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2266003982
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
The current server-side cache assumes the following race conditions. Say we have two HAed HMS instances X and Y with RANDOM load-balancing.
1. User A caches a table at T1 in HMS X
2. User B updates the same table to T2 and caches it in HMS Y
3. User C fetches the same table from HMS X, and HMS X returns the table at T1
I suppose the server-side eventual consistency requires us to design and advertise the consistency semantics carefully. I'd say the client side should do such a control.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2265924531
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
@henrib, can't we reuse `cachedTable` here instead of doing another HMS call?
````
location = cachedTable.getParameters().get(METADATA_LOCATION_PROP)
````
if not, why not reuse existing methods `hiveCatalog.loadTable(canonicalized).location();`
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2266091063
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
> I suppose the server-side eventual consistency requires us to design and advertise the consistency semantics carefully. I'd say the client side should do such a control.
@okumin, I think similar thing was discussed in https://lists.apache.org/thread/zb5ldjfxvjz4fjwll1lc9cm8rl015zx2
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29035: Fixing cache handling for REST catalog [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5882:
URL: https://github.com/apache/hive/pull/5882#discussion_r2266026469
##########
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:
##########
@@ -63,6 +69,48 @@ public List<Namespace> listNamespaces(Namespace nmspc) throws NoSuchNamespaceExc
return hiveCatalog.listNamespaces(nmspc);
}
+ @Override
+ public Table loadTable(TableIdentifier identifier) {
+ final Cache<TableIdentifier, Table> cache = this.tableCache;
+ final HiveCatalog catalog = this.hiveCatalog;
+ final TableIdentifier canonicalized = identifier.toLowerCase();
+ Table cachedTable = cache.getIfPresent(canonicalized);
+ if (cachedTable != null) {
+ String location = catalog.getTableLocation(canonicalized);
Review Comment:
location objects won't ever match !!!
one returns metadata file location and another just the root table location
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-29016: fixing cache handling for REST catalog; [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5882:
URL: https://github.com/apache/hive/pull/5882#issuecomment-3046256617
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5882&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5882&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5882&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5882)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org