[ 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)