roryqi commented on code in PR #11364:
URL: https://github.com/apache/gravitino/pull/11364#discussion_r3345915759
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java:
##########
@@ -76,6 +79,7 @@ public class RESTService implements GravitinoAuxiliaryService
{
private IcebergCatalogWrapperManager icebergCatalogWrapperManager;
private IcebergMetricsManager icebergMetricsManager;
+ private Optional<IcebergCleanupManager> cleanupManager = Optional.empty();
Review Comment:
Done — removed the redundant initializer in 28a283881. `serviceInit()`
assigns `cleanupManager` in both the auxiliary-mode and standalone branches
before any use, matching the sibling manager fields.
##########
iceberg/iceberg-rest-server/src/main/resources/META-INF/services/org.apache.gravitino.storage.relational.mapper.provider.MapperPackageProvider:
##########
@@ -1,19 +0,0 @@
-#
-# 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.
-#
-org.apache.gravitino.iceberg.service.cleanup.mapper.provider.IcebergCleanupMapperPackageProvider
Review Comment:
This is the `ServiceLoader` SPI registration, and it never actually worked
for our case. Core builds the shared `SqlSessionFactory` during startup and
runs `ServiceLoader.load(MapperPackageProvider.class)` in **core's** class
loader; in deploy mode the iceberg-rest-server runs in an isolated
auxiliary-service class loader, so this `META-INF/services` entry is invisible
to that lookup. The mapper was therefore never discovered — that is exactly the
`BindingException` (`IcebergCleanupJobMapper is not known to the
MapperRegistry`) the PR's CI hit.
We now register the mapper directly from `IcebergCleanupJobStore`'s
constructor, which runs inside the isolated class loader that can see the
mapper, and that path covers both deploy and single-class-loader unit tests. So
this SPI entry became dead code and was removed. I kept the
`IcebergCleanupMapperPackageProvider` class itself as the single source for the
mapper list.
If you'd rather keep the file, I can restore it, but it would be a no-op
entry implying a discovery mechanism that does not apply 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
