Uploaded image for project: 'Apache Twill'
  1. Apache Twill
  2. TWILL-223

FileContextLocationFactory should use FileContext instance based on the caller UGI

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11.0
    • Component/s: None
    • Labels:
      None

      Description

      The FileContextLocationFactory internally has a cached FileContext object that was created when the factory was created. However, when the getFileContext is called (directly or via one of those create methods), the UGI might be different then the cached one. If the cached one is returning, FS operations will be performed with the user who creates the factory, not the one who calls the create method.

        Issue Links

          Activity

          Hide
          chtyim Terence Yim added a comment -

          With this enhancement, the same FileContextLocationFactory can be reused for different user, hence allowing submitting application with different UGI via the same TwillRunner.

          Show
          chtyim Terence Yim added a comment - With this enhancement, the same FileContextLocationFactory can be reused for different user, hence allowing submitting application with different UGI via the same TwillRunner .
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user chtyim opened a pull request:

          https://github.com/apache/twill/pull/47

          (TWILL-223) Make FileContextLocationFactory UGI aware

          • Use different FileContext object based on the caller UGI
          • Allows sharing the same factory instance for different user

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/chtyim/twill feature/TWILL-223

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/twill/pull/47.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #47


          commit 3472cf55c9a7de912997c4e8f567a1192f8f8823
          Author: Terence Yim <chtyim@apache.org>
          Date: 2017-03-28T19:53:43Z

          (TWILL-223) Make FileContextLocationFactory UGI aware

          • Use different FileContext object based on the caller UGI
          • Allows sharing the same factory instance for different user

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user chtyim opened a pull request: https://github.com/apache/twill/pull/47 ( TWILL-223 ) Make FileContextLocationFactory UGI aware Use different FileContext object based on the caller UGI Allows sharing the same factory instance for different user You can merge this pull request into a Git repository by running: $ git pull https://github.com/chtyim/twill feature/ TWILL-223 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/47.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #47 commit 3472cf55c9a7de912997c4e8f567a1192f8f8823 Author: Terence Yim <chtyim@apache.org> Date: 2017-03-28T19:53:43Z ( TWILL-223 ) Make FileContextLocationFactory UGI aware Use different FileContext object based on the caller UGI Allows sharing the same factory instance for different user
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user albertshau commented on a diff in the pull request:

          https://github.com/apache/twill/pull/47#discussion_r108811273

          — Diff: twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocationFactory.java —
          @@ -103,16 +138,32 @@ public Location create(URI uri) {

          @Override
          public Location getHomeLocation()

          { + FileContext fc = getFileContext(); // Fix for TWILL-163. FileContext.getHomeDirectory() uses System.getProperty("user.name") instead of UGI return new FileContextLocation(this, fc, new Path(fc.getHomeDirectory().getParent(), fc.getUgi().getShortUserName())); }

          /**

          • * Returns the {@link FileContext} used by this {@link LocationFactory}.
            + * Returns the {@link FileContext}

            for the current user based on

            {@link UserGroupInformation#getCurrentUser()}

            .
            + *
            + * @throws IllegalStateException if failed to determine the current user or fail to create the FileContext.
            + * @throws RuntimeException if failed to get the

            {@link FileContext}

            object for the current user due to exception
            */
            public FileContext getFileContext() {

          • return fc;
            + try { + return fileContextCache.getUnchecked(UserGroupInformation.getCurrentUser()); + }

            catch (IOException e)

            { + throw new IllegalStateException("Failed to get current user information", e); + }

            catch (UncheckedExecutionException e) {
            + Throwable cause = e.getCause();
            + if (cause instanceof UnsupportedFileSystemException)

            { + String defaultURI = configuration.get(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, + CommonConfigurationKeysPublic.FS_DEFAULT_NAME_DEFAULT); + throw new IllegalStateException("File system with URI '" + defaultURI + "' is not supported", cause); + }

            + throw new RuntimeException(cause);

              • End diff –

          at this point the cause should be a RuntimeException right? in which case we should just propagate it directly rather than wrapping it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user albertshau commented on a diff in the pull request: https://github.com/apache/twill/pull/47#discussion_r108811273 — Diff: twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocationFactory.java — @@ -103,16 +138,32 @@ public Location create(URI uri) { @Override public Location getHomeLocation() { + FileContext fc = getFileContext(); // Fix for TWILL-163. FileContext.getHomeDirectory() uses System.getProperty("user.name") instead of UGI return new FileContextLocation(this, fc, new Path(fc.getHomeDirectory().getParent(), fc.getUgi().getShortUserName())); } /** * Returns the {@link FileContext} used by this {@link LocationFactory}. + * Returns the {@link FileContext} for the current user based on {@link UserGroupInformation#getCurrentUser()} . + * + * @throws IllegalStateException if failed to determine the current user or fail to create the FileContext. + * @throws RuntimeException if failed to get the {@link FileContext} object for the current user due to exception */ public FileContext getFileContext() { return fc; + try { + return fileContextCache.getUnchecked(UserGroupInformation.getCurrentUser()); + } catch (IOException e) { + throw new IllegalStateException("Failed to get current user information", e); + } catch (UncheckedExecutionException e) { + Throwable cause = e.getCause(); + if (cause instanceof UnsupportedFileSystemException) { + String defaultURI = configuration.get(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, + CommonConfigurationKeysPublic.FS_DEFAULT_NAME_DEFAULT); + throw new IllegalStateException("File system with URI '" + defaultURI + "' is not supported", cause); + } + throw new RuntimeException(cause); End diff – at this point the cause should be a RuntimeException right? in which case we should just propagate it directly rather than wrapping it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/47#discussion_r108826044

          — Diff: twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocationFactory.java —
          @@ -103,16 +138,32 @@ public Location create(URI uri) {

          @Override
          public Location getHomeLocation()

          { + FileContext fc = getFileContext(); // Fix for TWILL-163. FileContext.getHomeDirectory() uses System.getProperty("user.name") instead of UGI return new FileContextLocation(this, fc, new Path(fc.getHomeDirectory().getParent(), fc.getUgi().getShortUserName())); }

          /**

          • * Returns the {@link FileContext} used by this {@link LocationFactory}.
            + * Returns the {@link FileContext}

            for the current user based on

            {@link UserGroupInformation#getCurrentUser()}

            .
            + *
            + * @throws IllegalStateException if failed to determine the current user or fail to create the FileContext.
            + * @throws RuntimeException if failed to get the

            {@link FileContext}

            object for the current user due to exception
            */
            public FileContext getFileContext() {

          • return fc;
            + try { + return fileContextCache.getUnchecked(UserGroupInformation.getCurrentUser()); + }

            catch (IOException e)

            { + throw new IllegalStateException("Failed to get current user information", e); + }

            catch (UncheckedExecutionException e) {
            + Throwable cause = e.getCause();
            + if (cause instanceof UnsupportedFileSystemException)

            { + String defaultURI = configuration.get(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, + CommonConfigurationKeysPublic.FS_DEFAULT_NAME_DEFAULT); + throw new IllegalStateException("File system with URI '" + defaultURI + "' is not supported", cause); + }

            + throw new RuntimeException(cause);

              • End diff –

          Well, it can be `Exception` or `RuntimeException`. I can add a check to just throw the cause without wrapping if the type is `RuntimeException`

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/47#discussion_r108826044 — Diff: twill-yarn/src/main/java/org/apache/twill/filesystem/FileContextLocationFactory.java — @@ -103,16 +138,32 @@ public Location create(URI uri) { @Override public Location getHomeLocation() { + FileContext fc = getFileContext(); // Fix for TWILL-163. FileContext.getHomeDirectory() uses System.getProperty("user.name") instead of UGI return new FileContextLocation(this, fc, new Path(fc.getHomeDirectory().getParent(), fc.getUgi().getShortUserName())); } /** * Returns the {@link FileContext} used by this {@link LocationFactory}. + * Returns the {@link FileContext} for the current user based on {@link UserGroupInformation#getCurrentUser()} . + * + * @throws IllegalStateException if failed to determine the current user or fail to create the FileContext. + * @throws RuntimeException if failed to get the {@link FileContext} object for the current user due to exception */ public FileContext getFileContext() { return fc; + try { + return fileContextCache.getUnchecked(UserGroupInformation.getCurrentUser()); + } catch (IOException e) { + throw new IllegalStateException("Failed to get current user information", e); + } catch (UncheckedExecutionException e) { + Throwable cause = e.getCause(); + if (cause instanceof UnsupportedFileSystemException) { + String defaultURI = configuration.get(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, + CommonConfigurationKeysPublic.FS_DEFAULT_NAME_DEFAULT); + throw new IllegalStateException("File system with URI '" + defaultURI + "' is not supported", cause); + } + throw new RuntimeException(cause); End diff – Well, it can be `Exception` or `RuntimeException`. I can add a check to just throw the cause without wrapping if the type is `RuntimeException`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/47

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/47

            People

            • Assignee:
              chtyim Terence Yim
              Reporter:
              chtyim Terence Yim
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development