Copilot commented on code in PR #11060:
URL: https://github.com/apache/gravitino/pull/11060#discussion_r3227635381
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -115,6 +113,7 @@ protected Application configure() {
resourceConfig.property(CommonProperties.FEATURE_AUTO_DISCOVERY_DISABLE,
true);
resourceConfig.property(CommonProperties.MOXY_JSON_FEATURE_DISABLE, true);
ObjectMapper mapper = new ObjectMapper();
+ mapper.registerModule(new
org.openapitools.jackson.nullable.JsonNullableModule());
JacksonJaxbJsonProvider provider = new JacksonJaxbJsonProvider();
Review Comment:
Avoid using a fully-qualified class name inside the method body; import
JsonNullableModule and reference it directly to match the project's Java
import/style guidelines.
##########
core/src/main/java/org/apache/gravitino/stats/storage/LancePartitionStatisticStorage.java:
##########
@@ -204,10 +205,14 @@ public LancePartitionStatisticStorage(Map<String, String>
properties) {
.maximumSize(datasetCacheSize)
.scheduler(Scheduler.forScheduledExecutorService(this.scheduler))
.evictionListener(
- (RemovalListener<Long, Dataset>)
+ (RemovalListener<Long, DatasetHolder>)
Review Comment:
The cache uses Caffeine's evictionListener, which (per Caffeine semantics)
only runs for evictions (e.g., SIZE/EXPIRED) and not for removals like
REPLACED/EXPLICIT. Since this cache updates entries via put() (replacing the
previous DatasetHolder), the old Dataset may not be closed, potentially leaking
native/allocator resources. Prefer using a removalListener (or explicitly
closing the previous value on replacement) so replacements are cleaned up too.
##########
core/src/main/java/org/apache/gravitino/stats/storage/LancePartitionStatisticStorage.java:
##########
@@ -306,7 +311,7 @@ private void appendStatisticsImpl(Long tableId,
List<PartitionStatisticsUpdate>
newDataset = appendTxn.commit();
Dataset finalNewDataset = newDataset;
- datasetCache.ifPresent(cache -> cache.put(tableId, finalNewDataset));
+ datasetCache.ifPresent(cache -> cache.put(tableId, new
DatasetHolder(finalNewDataset)));
Review Comment:
This put() replaces the existing cached entry for the same tableId (the
initial DatasetHolder was inserted by cache.get(...)). Ensure the previous
cached DatasetHolder is closed on replacement (e.g., via a Caffeine
removalListener or by explicitly closing the old value) to avoid leaking native
resources.
##########
core/src/main/java/org/apache/gravitino/stats/storage/LancePartitionStatisticStorage.java:
##########
@@ -355,10 +360,10 @@ private void dropStatisticsImpl(Long tableId,
List<PartitionStatisticsDrop> drop
@Override
public void close() throws IOException {
if (datasetCache.isPresent()) {
- Cache<Long, Dataset> cache = datasetCache.get();
- for (Dataset dataset : cache.asMap().values()) {
+ Cache<Long, DatasetHolder> cache = datasetCache.get();
+ for (DatasetHolder holder : cache.asMap().values()) {
try {
- dataset.close();
+ holder.close();
} catch (Exception e) {
Review Comment:
Catching generic Exception here is overly broad and can mask unexpected
shutdown failures. Consider narrowing this to IOException (and optionally
RuntimeException) when closing cached datasets, especially if you switch to a
Caffeine removalListener where exceptions can be handled consistently in one
place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
