HIVE-29035 : new version of cache #6441
Conversation
…e DB to ensure no stale table object is returned;
- improved check; - quiesce console logs due to internal throws in servlet;
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
|
@ayushtkn if you have some spare time to review, thanks |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asf-ci-hive
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| HttpResponse<String> response; | ||
| try (HttpClient client = HttpClient.newHttpClient()) { | ||
| response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
| } |
asf-ci-hive
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
|
@henrib Could you please rebase this branch? My update introduced a conflict. Thanks. |
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
|
|
@henrib there are some warnings from Sonar & I feel few could be fixed |
| public class HMSCatalogAdapter implements Closeable { | ||
| private static final Logger LOG = LoggerFactory.getLogger(HMSCatalogAdapter.class); | ||
| private static final Splitter SLASH = Splitter.on('/'); | ||
| private static final String V1_CACHE_STATS = "v1/cache/stats"; |
There was a problem hiding this comment.
I may want to add prefix for consistency and just in case for multi-tenant features. As another point, I am a bit concerned about the naming; as cache is generic, it might conflict in the future.
I'm wondering if we can expose the metrics via JMX rather than a RESTful endpoint. It allows us to remove the endpoint for mostly testing purposes, disallows normal users to access the metrics, and allows administrators to see the metrics via Datadog easily.
| String tableName = baseTableIdentifier.name(); | ||
| try { | ||
| List<Table> tables = clients.run( | ||
| client -> client.getTables(catName, database, Collections.singletonList(tableName), PARAM_SPEC) |
There was a problem hiding this comment.
TODO: It is possible to use IMetaStoreClient#getTable, but #getTables with PARAM_SPEC could be more lightweight. Which is better?
| try { | ||
| List<Table> tables = clients.run( | ||
| client -> client.getTables(catName, database, Collections.singletonList(tableName), PARAM_SPEC) | ||
| ); |
There was a problem hiding this comment.
Should we run HiveOperationsBase.validateTableIsIceberg(table, fullName); explicitly?
| return tables == null || tables.isEmpty() | ||
| ? null | ||
| : tables.getFirst().getParameters().get(BaseMetastoreTableOperations.METADATA_LOCATION_PROP); | ||
| } catch (NoSuchTableException e) { |
There was a problem hiding this comment.
If I understand correctly, this is not thrown here.
| LOGGER.debug("Table {} not found: {}", baseTableIdentifier, e.getMessage()); | ||
| throw e; | ||
| } catch (NoSuchObjectException e) { | ||
| throw new NoSuchTableException("Table %s not found: %s", baseTableIdentifier, e.getMessage()); |
There was a problem hiding this comment.
This might also not happen since we obtain the list of tables. Also, this method can either return null or throw NoSuchTableException. We may use only either.
| // A RESTException is thrown by HMSCatalogAdapter.execute() after the error handler has | ||
| // already written the correct HTTP status and body to the response (e.g. 404, 403). | ||
| // It is not an unexpected server failure, so log at DEBUG to avoid flooding the console. | ||
| LOG.debug("REST request resulted in a client error (already handled): {}", e.getMessage()); |
There was a problem hiding this comment.
We may not need this one now
|
Please also address issues reported by SonarQube. |
This checks the table location from HMS DB to ensure no stale table object is returned;