wypoon commented on code in PR #12721:
URL: https://github.com/apache/iceberg/pull/12721#discussion_r2033957073


##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveMetastore.java:
##########
@@ -186,9 +189,9 @@ public void stop() throws Exception {
     if (executorService != null) {
       executorService.shutdown();
     }
-    if (baseHandler != null) {
-      baseHandler.shutdown();
-    }
+    // if (baseHandler != null) {
+    //   baseHandler.shutdown();
+    // }

Review Comment:
   @pvary you did not double post.
   
   The problem is that `TestHiveMetastore` has a field that is set in 
`newThriftServer` and whose `shutdown` method is called in `stop`. We need to 
declare this field. The question is: what type should we declare it to be? This 
`shutdown` method is in `FacebookBase`/`FacebookService.Iface`. But in 
`newThriftServer`, the field is also used to obtain an `IHMSHandler` through
   ```
       IHMSHandler handler = GET_BASE_HMS_HANDLER.invoke(serverConf, 
baseHandler, false);
   ```
   The type that is needed in that method invocation is `IHMSHandler`.
   So that is what I mean: we need something that is on one hand, an 
`IHMSHandler`, and on the other hand, a `FacebookBase`/`FacebookService.Iface`. 
But there is no common parent interface of both. I.e., there is no type `Foo` 
that I can use to declare
   ```
     private Foo baseHandler;
     ...
       if (baseHandler != null) {
         baseHandler.shutdown();
       }
     ...
       baseHandler = HMS_HANDLER_CTOR.newInstance("new db based metaserver", 
serverConf);
       IHMSHandler handler = GET_BASE_HMS_HANDLER.invoke(serverConf, 
baseHandler, false);
   ```
   
   To workaround that, I declare a field of type `FacebookBase` and set it in 
`newThriftServer` to be the `IHMSHandler` instantiated by 
`HMS_HANDLER_CTOR.newInstance("new db based metaserver", serverConf)`. But to 
set it, I need to check that it is an instance of the type the field is 
declared to be before casting it.
   > Maybe we just should skip the instanceof check?
   
   FIrst, that would not be safe. Second, how does that help?
   



-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to