[GitHub] [hudi] vinothchandar commented on a change in pull request #2899: [HUDI-1865] Make embedded time line service singleton

[GitBox <gi...@apache.org>]

vinothchandar commented on a change in pull request #2899:
URL: https://github.com/apache/hudi/pull/2899#discussion_r630455592



##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/embedded/EmbeddedTimelineServerHelper.java
##########
@@ -35,16 +35,23 @@
 
   private static final Logger LOG = LogManager.getLogger(EmbeddedTimelineService.class);
 
+  private static Option<EmbeddedTimelineService> timelineServer;
+
   /**
    * Instantiate Embedded Timeline Server.
    * @param context Hoodie Engine Context
    * @param config     Hoodie Write Config
    * @return TimelineServer if configured to run
    * @throws IOException
    */
-  public static Option<EmbeddedTimelineService> createEmbeddedTimelineService(
+  public static synchronized Option<EmbeddedTimelineService> createEmbeddedTimelineService(
       HoodieEngineContext context, HoodieWriteConfig config) throws IOException {
-    Option<EmbeddedTimelineService> timelineServer = Option.empty();
+    if (timelineServer != null

Review comment:
       I feel we can just init `timelineServer = Option.empty`. This will avoid the null check here. and also the else block down below. 

##########
File path: hudi-common/src/main/java/org/apache/hudi/common/table/view/FileSystemViewStorageConfig.java
##########
@@ -86,6 +86,12 @@
   // Need to be disabled only for tests.
   public static final String DEFAULT_REMOTE_BACKUP_VIEW_HANDLER_ENABLE = "true";
 
+  /**
+   * Configs to control whether to make the embedded timeline service singleton.
+   */
+  public static final String FILESYSTEM_VIEW_SINGLETON = "hoodie.filesystem.view.singleton.enable";

Review comment:
       I think it's better to keep this config with the ones that enable the timeline server for e.g `hoodie.embed.timeline.server` is in `HoodieWriteConfig`.  Can we add a `hoodie.timeline.server.reuse.enabled` there?

##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/embedded/EmbeddedTimelineService.java
##########
@@ -113,6 +113,12 @@ public FileSystemViewManager getViewManager() {
     return viewManager;
   }
 
+  public boolean canReuse(String basePath) {

Review comment:
       rename: canReuseFor() (just a suggestion, your 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.

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