hawq-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From denalex <...@git.apache.org>
Subject [GitHub] incubator-hawq pull request #1379: WIP: Cache UGI objects and clean them per...
Date Tue, 03 Jul 2018 21:16:34 GMT
Github user denalex commented on a diff in the pull request:

    https://github.com/apache/incubator-hawq/pull/1379#discussion_r199952336
  
    --- Diff: pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java
---
    @@ -89,32 +182,98 @@ public Boolean run() throws IOException, ServletException {
                 };
     
                 // create proxy user UGI from the UGI of the logged in user and execute the
servlet chain as that user
    -            UserGroupInformation proxyUGI = null;
    +            TimedProxyUGI timedProxyUGI = getTimedProxyUGI(user, session);
                 try {
    -                LOG.debug("Creating proxy user = " + user);
    -                proxyUGI = UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser());
    -                proxyUGI.doAs(action);
    +                timedProxyUGI.proxyUGI.doAs(action);
                 } catch (UndeclaredThrowableException ute) {
                     // unwrap the real exception thrown by the action
                     throw new ServletException(ute.getCause());
                 } catch (InterruptedException ie) {
                     throw new ServletException(ie);
    -            } finally {
    -                try {
    -                    if (proxyUGI != null) {
    -                        LOG.debug("Closing FileSystem for proxy user = " + proxyUGI.getUserName());
    -                        FileSystem.closeAllForUGI(proxyUGI);
    -                    }
    -                } catch (Throwable t) {
    -                    LOG.warn("Error closing FileSystem for proxy user = " + proxyUGI.getUserName());
    -                }
    +            }
    +            finally {
    +                release(timedProxyUGI, fragmentIndex, fragmentCount);
                 }
             } else {
                 // no user impersonation is configured
                 chain.doFilter(request, response);
             }
         }
     
    +   private TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session)
throws IOException {
    +        synchronized (session.segmentTransactionId.intern()) {
    --- End diff --
    
    synchronizing on interned strings might not be a good practice (https://stackoverflow.com/questions/133988/synchronizing-on-string-objects-in-java),
basically due to:
    - possibilities that other logic elsewhere does it as well and creates possibility of
deadlock
    - intern() has cost and might require internal synchronization
    - static pool of interned instances will grow and then GC will reclaim them, so unclear
lifecycle of objects
    It would seem preferable to use local object we have full control over


---

Mime
View raw message