[ 
https://issues.apache.org/jira/browse/GOBBLIN-1136?focusedWorklogId=429356&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-429356
 ]

ASF GitHub Bot logged work on GOBBLIN-1136:
-------------------------------------------

                Author: ASF GitHub Bot
            Created on: 01/May/20 04:18
            Start Date: 01/May/20 04:18
    Worklog Time Spent: 10m 
      Work Description: sv2000 commented on a change in pull request #2975:
URL: https://github.com/apache/incubator-gobblin/pull/2975#discussion_r418407392



##########
File path: 
gobblin-utility/src/main/java/org/apache/gobblin/util/logs/LogCopier.java
##########
@@ -271,7 +282,23 @@ void checkSrcLogFiles() throws IOException {
         LOGGER.error("Failed LogCopyTask - {}", e);
       }
     }
+    if (needToUpdateDestFs) {
+      if(destFsSupplier == null) {
+        throw new IOException("Try to update dest fileSystem but 
destFsSupplier has not been set, there might be something wrong in code");

Review comment:
       You can drop "there might be something wrong in code" part.

##########
File path: 
gobblin-utility/src/main/java/org/apache/gobblin/util/logs/LogCopier.java
##########
@@ -271,7 +282,23 @@ void checkSrcLogFiles() throws IOException {
         LOGGER.error("Failed LogCopyTask - {}", e);
       }
     }
+    if (needToUpdateDestFs) {
+      if(destFsSupplier == null) {
+        throw new IOException("Try to update dest fileSystem but 
destFsSupplier has not been set, there might be something wrong in code");
+      }
+      this.destFs.close();
+      this.destFs = destFsSupplier.getFileSystem();
+      LOGGER.info("Dest fs updated" + destFs.toString());
 
+    }
+    if (needToUpdateSrcFs) {
+      if(srcFsSupplier == null) {
+        throw new IOException("Try to update source fileSystem but 
srcFsSupplier has not been set, there might be something wrong in code");

Review comment:
       same comment as earlier.

##########
File path: 
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/YarnContainerSecurityManager.java
##########
@@ -58,20 +59,27 @@
   private final FileSystem fs;
   private final Path tokenFilePath;
   private final EventBus eventBus;
+  private final LogCopier logCopier;
 
   public YarnContainerSecurityManager(Config config, FileSystem fs, EventBus 
eventBus) {
+    this(config, fs, eventBus, null);
+  }
+
+  public YarnContainerSecurityManager(Config config, FileSystem fs, EventBus 
eventBus, LogCopier logCopier) {
     this.fs = fs;
     this.tokenFilePath = new Path(this.fs.getHomeDirectory(),
         config.getString(GobblinYarnConfigurationKeys.APPLICATION_NAME_KEY) + 
Path.SEPARATOR
             + GobblinYarnConfigurationKeys.TOKEN_FILE_NAME);
     this.eventBus = eventBus;
+    this.logCopier = logCopier;
   }
 
   @SuppressWarnings("unused")
   @Subscribe
   public void handleTokenFileUpdatedEvent(DelegationTokenUpdatedEvent 
delegationTokenUpdatedEvent) {
     try {
       addCredentials(readCredentials(this.tokenFilePath));
+      this.logCopier.setNeedToUpdateDestFs(true);

Review comment:
       A null check looks necessary here. 

##########
File path: 
gobblin-utility/src/main/java/org/apache/gobblin/util/logs/LogCopier.java
##########
@@ -271,7 +282,23 @@ void checkSrcLogFiles() throws IOException {
         LOGGER.error("Failed LogCopyTask - {}", e);
       }
     }
+    if (needToUpdateDestFs) {
+      if(destFsSupplier == null) {
+        throw new IOException("Try to update dest fileSystem but 
destFsSupplier has not been set, there might be something wrong in code");
+      }
+      this.destFs.close();
+      this.destFs = destFsSupplier.getFileSystem();
+      LOGGER.info("Dest fs updated" + destFs.toString());
 
+    }
+    if (needToUpdateSrcFs) {

Review comment:
       Does this boolean need to be reset back to false after the logic in the 
if {..} block has finished?

##########
File path: 
gobblin-utility/src/main/java/org/apache/gobblin/util/logs/LogCopier.java
##########
@@ -337,6 +366,30 @@ public Builder useTimeUnit(TimeUnit timeUnit) {
       return this;
     }
 
+    /**
+     * Set the {@link FileSystemSupplier} used for generating new Dest 
FileSystem later when token been updated.
+     *
+     * @param supplier the {@link FileSystemSupplier} used for generating new 
Dest FileSystem
+     * @return this {@link LogCopier.Builder} instance
+     */
+    public Builder useDestFsSupplier(FileSystemSupplier supplier) {

Review comment:
       Do we need a separate src and destination FS supplier? 

##########
File path: 
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnLogSource.java
##########
@@ -76,6 +77,12 @@ protected LogCopier buildLogCopier(Config config, String 
containerId, FileSystem
     LogCopier.Builder builder = LogCopier.newBuilder()
             .useSrcFileSystem(buildFileSystem(config, true))

Review comment:
       Should we replace this with the FsSupplier?

##########
File path: 
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnLogSource.java
##########
@@ -76,6 +77,12 @@ protected LogCopier buildLogCopier(Config config, String 
containerId, FileSystem
     LogCopier.Builder builder = LogCopier.newBuilder()
             .useSrcFileSystem(buildFileSystem(config, true))
             .useDestFileSystem(buildFileSystem(config, false))

Review comment:
       same comment as earlier.




----------------------------------------------------------------
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:
us...@infra.apache.org


Issue Time Tracking
-------------------

    Worklog Id:     (was: 429356)
    Time Spent: 20m  (was: 10m)

> Make LogCopier be able to refresh FileSystem for long running job use cases
> ---------------------------------------------------------------------------
>
>                 Key: GOBBLIN-1136
>                 URL: https://issues.apache.org/jira/browse/GOBBLIN-1136
>             Project: Apache Gobblin
>          Issue Type: Task
>            Reporter: Zihan Li
>            Priority: Major
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> Make LogCopier be able to refresh FileSystem for long running job use cases



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to