From b05aa204c0ed7f6d0adbd90c0ceff2a97dad6fb4 Mon Sep 17 00:00:00 2001 From: lavjain Date: Fri, 29 Jun 2018 09:34:48 -0700 Subject: [PATCH 01/28] WIP: Cache UGI objects and clean them periodically --- .../servlet/SecurityServletFilter.java | 124 ++++++++++++++---- 1 file changed, 99 insertions(+), 25 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 4cf3d0379b..1149e8394d 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -23,15 +23,25 @@ import java.io.IOException; import java.lang.reflect.UndeclaredThrowableException; import java.security.PrivilegedExceptionAction; - -import javax.servlet.*; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.DelayQueue; +import java.util.concurrent.Delayed; +import java.util.concurrent.TimeUnit; + +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.util.Time; import org.apache.hawq.pxf.service.utilities.SecureLogin; @@ -42,8 +52,51 @@ public class SecurityServletFilter implements Filter { private static final Log LOG = LogFactory.getLog(SecurityServletFilter.class); private static final String USER_HEADER = "X-GP-USER"; - private static final String MISSING_HEADER_ERROR = String.format("Header %s is missing in the request", USER_HEADER); - private static final String EMPTY_HEADER_ERROR = String.format("Header %s is empty in the request", USER_HEADER); + private static final String SEGMENT_INDEX_HEADER = "X-GP-SEGMENT-ID"; + private static final String TRANSACTION_ID_HEADER = "X-GP-XID"; + private static final String MISSING_HEADER_ERROR = "Header %s is missing in the request"; + private static final String EMPTY_HEADER_ERROR = "Header %s is empty in the request"; + private static Map cache = new ConcurrentHashMap<>(); + private static DelayQueue delayQueue = new DelayQueue<>(); + private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes + + + private static class TimedProxyUGI implements Delayed { + long startTime; + UserGroupInformation proxyUGI; + SegmentTransactionId session; + + public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session) { + this.startTime = System.currentTimeMillis(); + this.proxyUGI = proxyUGI; + this.session = session; + } + + @Override + public long getDelay(TimeUnit unit) { + return unit.convert(getDelayMillis(), TimeUnit.MILLISECONDS); + } + + @Override + public int compareTo(Delayed other) { + return Long.compare(this.getDelayMillis(), ((TimedProxyUGI)other).getDelayMillis()); + } + + private long getDelayMillis() { + return (startTime + UGI_CACHE_EXPIRY) - System.currentTimeMillis(); + } + } + + private static class SegmentTransactionId { + String segmentIndex; + String transactionId; + + public SegmentTransactionId(String segmentIndex, String transactionId) { + this.segmentIndex = segmentIndex; + this.transactionId = transactionId; + } + + } /** * Initializes the filter. @@ -64,17 +117,15 @@ public void init(FilterConfig filterConfig) throws ServletException { * @param chain filter chain */ @Override - public void doFilter(final ServletRequest request, final ServletResponse response, final FilterChain chain) throws IOException, ServletException { + public void doFilter(final ServletRequest request, final ServletResponse response, final FilterChain chain) + throws IOException, ServletException { if (SecureLogin.isUserImpersonationEnabled()) { // retrieve user header and make sure header is present and is not empty - final String user = ((HttpServletRequest) request).getHeader(USER_HEADER); - if (user == null) { - throw new IllegalArgumentException(MISSING_HEADER_ERROR); - } else if (user.trim().isEmpty()) { - throw new IllegalArgumentException(EMPTY_HEADER_ERROR); - } + final String user = getHeaderValue(request, USER_HEADER); + final String segmentIndex = getHeaderValue(request, SEGMENT_INDEX_HEADER); + final String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER); // TODO refresh Kerberos token when security is enabled @@ -89,25 +140,24 @@ 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; - try { + SegmentTransactionId session = new SegmentTransactionId(segmentIndex, transactionId); + TimedProxyUGI timedProxyUGI = cache.get(session); + if (timedProxyUGI == null || timedProxyUGI.getDelayMillis() > 0) { + cleanup(); LOG.debug("Creating proxy user = " + user); - proxyUGI = UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); - proxyUGI.doAs(action); + UserGroupInformation proxyUGI = + UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); + timedProxyUGI = new TimedProxyUGI(proxyUGI, session); + delayQueue.offer(timedProxyUGI); + cache.put(session, timedProxyUGI); + } + try { + 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()); - } } } else { // no user impersonation is configured @@ -115,6 +165,30 @@ public Boolean run() throws IOException, ServletException { } } + private void cleanup() { + TimedProxyUGI timedProxyUGI = delayQueue.poll(); + while (timedProxyUGI != null) { + try { + LOG.debug("Closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); + FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); + cache.remove(timedProxyUGI.session); + } catch (Throwable t) { + LOG.warn("Error closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); + } + timedProxyUGI = delayQueue.poll(); + } + } + + private String getHeaderValue(ServletRequest request, String headerKey) throws IllegalArgumentException { + String value = ((HttpServletRequest) request).getHeader(headerKey); + if (value == null) { + throw new IllegalArgumentException(String.format(MISSING_HEADER_ERROR, headerKey)); + } else if (value.trim().isEmpty()) { + throw new IllegalArgumentException(String.format(EMPTY_HEADER_ERROR, headerKey)); + } + return value; + } + /** * Destroys the filter. */ From 2172b87c82ecfd1e2b2ad8fc78b8e0c61dba8f02 Mon Sep 17 00:00:00 2001 From: lavjain Date: Mon, 2 Jul 2018 13:40:03 -0700 Subject: [PATCH 02/28] Close UGI handle when it is the last fragment --- .../servlet/SecurityServletFilter.java | 109 ++++++++++++++---- 1 file changed, 84 insertions(+), 25 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 1149e8394d..bad559c409 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -52,8 +52,11 @@ public class SecurityServletFilter implements Filter { private static final Log LOG = LogFactory.getLog(SecurityServletFilter.class); private static final String USER_HEADER = "X-GP-USER"; - private static final String SEGMENT_INDEX_HEADER = "X-GP-SEGMENT-ID"; + private static final String SEGMENT_COUNT_HEADER = "X-GP-SEGMENT-COUNT"; + private static final String SEGMENT_ID_HEADER = "X-GP-SEGMENT-ID"; private static final String TRANSACTION_ID_HEADER = "X-GP-XID"; + private static final String FRAGMENT_INDEX_HEADER = "X-GP-FRAGMENT-INDEX"; + private static final String FRAGMENT_COUNT_HEADER = "X-GP-FRAGMENT-COUNT"; private static final String MISSING_HEADER_ERROR = "Header %s is missing in the request"; private static final String EMPTY_HEADER_ERROR = "Header %s is empty in the request"; private static Map cache = new ConcurrentHashMap<>(); @@ -61,7 +64,7 @@ public class SecurityServletFilter implements Filter { private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes - private static class TimedProxyUGI implements Delayed { + private class TimedProxyUGI implements Delayed { long startTime; UserGroupInformation proxyUGI; SegmentTransactionId session; @@ -72,6 +75,10 @@ public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session this.session = session; } + public void resetTime() { + startTime = System.currentTimeMillis(); + } + @Override public long getDelay(TimeUnit unit) { return unit.convert(getDelayMillis(), TimeUnit.MILLISECONDS); @@ -79,7 +86,8 @@ public long getDelay(TimeUnit unit) { @Override public int compareTo(Delayed other) { - return Long.compare(this.getDelayMillis(), ((TimedProxyUGI)other).getDelayMillis()); + TimedProxyUGI that = (TimedProxyUGI) other; + return Long.compare(this.getDelayMillis(), that.getDelayMillis()); } private long getDelayMillis() { @@ -87,15 +95,32 @@ private long getDelayMillis() { } } - private static class SegmentTransactionId { - String segmentIndex; + private class SegmentTransactionId { + String segmentId; String transactionId; + String segmentTransactionId; + boolean inProgress; - public SegmentTransactionId(String segmentIndex, String transactionId) { - this.segmentIndex = segmentIndex; + public SegmentTransactionId(String segmentId, String transactionId) { + this.segmentId = segmentId; this.transactionId = transactionId; + this.segmentTransactionId = segmentId + ":" + transactionId; + inProgress = true; + } + + @Override + public int hashCode() { + return segmentTransactionId.hashCode(); } + @Override + public boolean equals(Object other) { + if (!(other instanceof SegmentTransactionId)) { + return false; + } + SegmentTransactionId that = (SegmentTransactionId) other; + return this.segmentTransactionId.equals(that.segmentTransactionId); + } } /** @@ -124,12 +149,22 @@ public void doFilter(final ServletRequest request, final ServletResponse respons // retrieve user header and make sure header is present and is not empty final String user = getHeaderValue(request, USER_HEADER); - final String segmentIndex = getHeaderValue(request, SEGMENT_INDEX_HEADER); - final String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER); + String segmentId = getHeaderValue(request, SEGMENT_ID_HEADER); + String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER); + Integer fragmentCount = getHeaderValueInt(request, FRAGMENT_COUNT_HEADER, false); + Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); + LOG.info("Session = " + segmentId + ":" + transactionId); + StringBuilder sb = new StringBuilder("Session = ") + .append(segmentId).append(":").append(transactionId); + if (fragmentCount != null) { + sb.append("Fragment = ").append(fragmentIndex).append(" of ").append(fragmentCount) + .append(" (SegmentId = ").append(segmentId).append(")"); + } + LOG.info(sb.toString()); // TODO refresh Kerberos token when security is enabled - // prepare pivileged action to run on behalf of proxy user + // prepare privileged action to run on behalf of proxy user PrivilegedExceptionAction action = new PrivilegedExceptionAction() { @Override public Boolean run() throws IOException, ServletException { @@ -140,11 +175,11 @@ 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 - SegmentTransactionId session = new SegmentTransactionId(segmentIndex, transactionId); + SegmentTransactionId session = new SegmentTransactionId(segmentId, transactionId); TimedProxyUGI timedProxyUGI = cache.get(session); - if (timedProxyUGI == null || timedProxyUGI.getDelayMillis() > 0) { + if (timedProxyUGI == null || timedProxyUGI.getDelayMillis() < 0) { cleanup(); - LOG.debug("Creating proxy user = " + user); + LOG.info("Creating proxy user = " + user); UserGroupInformation proxyUGI = UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); timedProxyUGI = new TimedProxyUGI(proxyUGI, session); @@ -159,6 +194,12 @@ public Boolean run() throws IOException, ServletException { } catch (InterruptedException ie) { throw new ServletException(ie); } + finally { + if (fragmentIndex != null && fragmentCount.equals(fragmentIndex)) + closeUGI(timedProxyUGI); + else + timedProxyUGI.resetTime(); + } } else { // no user impersonation is configured chain.doFilter(request, response); @@ -166,24 +207,42 @@ public Boolean run() throws IOException, ServletException { } private void cleanup() { - TimedProxyUGI timedProxyUGI = delayQueue.poll(); - while (timedProxyUGI != null) { - try { - LOG.debug("Closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); - FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); - cache.remove(timedProxyUGI.session); - } catch (Throwable t) { - LOG.warn("Error closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); + synchronized (delayQueue) { + TimedProxyUGI timedProxyUGI = delayQueue.poll(); + while (timedProxyUGI != null) { + closeUGI(timedProxyUGI); + timedProxyUGI = delayQueue.poll(); } - timedProxyUGI = delayQueue.poll(); } } - private String getHeaderValue(ServletRequest request, String headerKey) throws IllegalArgumentException { + private void closeUGI(TimedProxyUGI timedProxyUGI) { + try { + LOG.info("Closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); + FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); + cache.remove(timedProxyUGI.session); + } catch (Throwable t) { + LOG.info("Error closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); + } + } + + private Integer getHeaderValueInt(ServletRequest request, String headerKey, boolean required) + throws IllegalArgumentException { + String value = getHeaderValue(request, headerKey, required); + return value != null ? Integer.valueOf(value) : null; + } + + private String getHeaderValue(ServletRequest request, String headerKey) + throws IllegalArgumentException { + return getHeaderValue(request, headerKey, true); + } + + private String getHeaderValue(ServletRequest request, String headerKey, boolean required) + throws IllegalArgumentException { String value = ((HttpServletRequest) request).getHeader(headerKey); - if (value == null) { + if (required && value == null) { throw new IllegalArgumentException(String.format(MISSING_HEADER_ERROR, headerKey)); - } else if (value.trim().isEmpty()) { + } else if (required && value.trim().isEmpty()) { throw new IllegalArgumentException(String.format(EMPTY_HEADER_ERROR, headerKey)); } return value; From 30a7c3c528ffa976580eac75a4d209611b4c8571 Mon Sep 17 00:00:00 2001 From: lavjain Date: Tue, 3 Jul 2018 13:21:21 -0700 Subject: [PATCH 03/28] Implement inProgress for reference counting --- .../servlet/SecurityServletFilter.java | 96 ++++++++++++------- 1 file changed, 61 insertions(+), 35 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index bad559c409..869cb49811 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -24,6 +24,7 @@ import java.lang.reflect.UndeclaredThrowableException; import java.security.PrivilegedExceptionAction; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.DelayQueue; import java.util.concurrent.Delayed; @@ -68,15 +69,18 @@ private class TimedProxyUGI implements Delayed { long startTime; UserGroupInformation proxyUGI; SegmentTransactionId session; + AtomicInteger inProgress = new AtomicInteger(); public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session) { this.startTime = System.currentTimeMillis(); this.proxyUGI = proxyUGI; this.session = session; + inProgress.incrementAndGet(); } public void resetTime() { startTime = System.currentTimeMillis(); + inProgress.decrementAndGet(); } @Override @@ -99,13 +103,11 @@ private class SegmentTransactionId { String segmentId; String transactionId; String segmentTransactionId; - boolean inProgress; public SegmentTransactionId(String segmentId, String transactionId) { this.segmentId = segmentId; this.transactionId = transactionId; this.segmentTransactionId = segmentId + ":" + transactionId; - inProgress = true; } @Override @@ -121,6 +123,11 @@ public boolean equals(Object other) { SegmentTransactionId that = (SegmentTransactionId) other; return this.segmentTransactionId.equals(that.segmentTransactionId); } + + @Override + public String toString() { + return "Session = " + segmentTransactionId; + } } /** @@ -153,14 +160,14 @@ public void doFilter(final ServletRequest request, final ServletResponse respons String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER); Integer fragmentCount = getHeaderValueInt(request, FRAGMENT_COUNT_HEADER, false); Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); - LOG.info("Session = " + segmentId + ":" + transactionId); - StringBuilder sb = new StringBuilder("Session = ") - .append(segmentId).append(":").append(transactionId); + + SegmentTransactionId session = new SegmentTransactionId(segmentId, transactionId); if (fragmentCount != null) { - sb.append("Fragment = ").append(fragmentIndex).append(" of ").append(fragmentCount) - .append(" (SegmentId = ").append(segmentId).append(")"); + StringBuilder sb = new StringBuilder(session.toString()); + sb.append(" (Fragment = ").append(fragmentIndex) + .append(" of ").append(fragmentCount).append(")"); + LOG.info(sb.toString()); } - LOG.info(sb.toString()); // TODO refresh Kerberos token when security is enabled @@ -175,17 +182,7 @@ 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 - SegmentTransactionId session = new SegmentTransactionId(segmentId, transactionId); - TimedProxyUGI timedProxyUGI = cache.get(session); - if (timedProxyUGI == null || timedProxyUGI.getDelayMillis() < 0) { - cleanup(); - LOG.info("Creating proxy user = " + user); - UserGroupInformation proxyUGI = - UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); - timedProxyUGI = new TimedProxyUGI(proxyUGI, session); - delayQueue.offer(timedProxyUGI); - cache.put(session, timedProxyUGI); - } + TimedProxyUGI timedProxyUGI = getTimedProxyUGI(user, session); try { timedProxyUGI.proxyUGI.doAs(action); } catch (UndeclaredThrowableException ute) { @@ -195,10 +192,7 @@ public Boolean run() throws IOException, ServletException { throw new ServletException(ie); } finally { - if (fragmentIndex != null && fragmentCount.equals(fragmentIndex)) - closeUGI(timedProxyUGI); - else - timedProxyUGI.resetTime(); + release(timedProxyUGI, fragmentIndex, fragmentCount); } } else { // no user impersonation is configured @@ -206,23 +200,55 @@ public Boolean run() throws IOException, ServletException { } } - private void cleanup() { - synchronized (delayQueue) { - TimedProxyUGI timedProxyUGI = delayQueue.poll(); - while (timedProxyUGI != null) { - closeUGI(timedProxyUGI); - timedProxyUGI = delayQueue.poll(); + private TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) throws IOException { + synchronized (session.segmentTransactionId.intern()) { + TimedProxyUGI timedProxyUGI = cache.get(session); + if (timedProxyUGI == null || timedProxyUGI.getDelayMillis() < 0) { + cleanup(); + LOG.info(session.toString() + " Creating proxy user = " + user); + UserGroupInformation proxyUGI = + UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); + timedProxyUGI = new TimedProxyUGI(proxyUGI, session); + delayQueue.offer(timedProxyUGI); + cache.put(session, timedProxyUGI); + } else { + timedProxyUGI.inProgress.incrementAndGet(); } + return timedProxyUGI; + } + } + + private void release(TimedProxyUGI timedProxyUGI, Integer fragmentIndex, Integer fragmentCount) { + synchronized (timedProxyUGI.session.segmentTransactionId.intern()) { + timedProxyUGI.resetTime(); + if (fragmentIndex != null && fragmentCount.equals(fragmentIndex)) + closeUGI(timedProxyUGI); + } + } + + private void cleanup() { + TimedProxyUGI timedProxyUGI = delayQueue.poll(); + while (timedProxyUGI != null) { + closeUGI(timedProxyUGI); + timedProxyUGI = delayQueue.poll(); } } private void closeUGI(TimedProxyUGI timedProxyUGI) { - try { - LOG.info("Closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); - FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); - cache.remove(timedProxyUGI.session); - } catch (Throwable t) { - LOG.info("Error closing FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName()); + synchronized (timedProxyUGI.session.segmentTransactionId.intern()) { + String fsMsg = "FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName(); + try { + if (timedProxyUGI.inProgress.get() != 0) { + LOG.info(timedProxyUGI.session.toString() + " Skipping close of " + fsMsg); + return; + } + cache.remove(timedProxyUGI.session); + LOG.info(timedProxyUGI.session.toString() + " Closing " + fsMsg + + " (Cache Size = " + cache.size() + ")"); + FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); + } catch (Throwable t) { + LOG.warn(timedProxyUGI.session.toString() + " Error closing " + fsMsg); + } } } From 19418ad4faa1956f7bf2476d2997ab36e2c0e0a0 Mon Sep 17 00:00:00 2001 From: lavjain Date: Tue, 3 Jul 2018 14:20:58 -0700 Subject: [PATCH 04/28] Code working as expected --- .../servlet/SecurityServletFilter.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 869cb49811..b2279f92b2 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -78,9 +78,16 @@ public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session inProgress.incrementAndGet(); } + public void incrementCounter() { + inProgress.incrementAndGet(); + } + + public void decrementCounter() { + inProgress.decrementAndGet(); + } + public void resetTime() { startTime = System.currentTimeMillis(); - inProgress.decrementAndGet(); } @Override @@ -164,8 +171,7 @@ public void doFilter(final ServletRequest request, final ServletResponse respons SegmentTransactionId session = new SegmentTransactionId(segmentId, transactionId); if (fragmentCount != null) { StringBuilder sb = new StringBuilder(session.toString()); - sb.append(" (Fragment = ").append(fragmentIndex) - .append(" of ").append(fragmentCount).append(")"); + sb.append(" Fragment = ").append(fragmentIndex).append(" of ").append(fragmentCount); LOG.info(sb.toString()); } @@ -212,7 +218,7 @@ private TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session delayQueue.offer(timedProxyUGI); cache.put(session, timedProxyUGI); } else { - timedProxyUGI.inProgress.incrementAndGet(); + timedProxyUGI.incrementCounter(); } return timedProxyUGI; } @@ -221,6 +227,7 @@ private TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session private void release(TimedProxyUGI timedProxyUGI, Integer fragmentIndex, Integer fragmentCount) { synchronized (timedProxyUGI.session.segmentTransactionId.intern()) { timedProxyUGI.resetTime(); + timedProxyUGI.decrementCounter(); if (fragmentIndex != null && fragmentCount.equals(fragmentIndex)) closeUGI(timedProxyUGI); } @@ -230,6 +237,7 @@ private void cleanup() { TimedProxyUGI timedProxyUGI = delayQueue.poll(); while (timedProxyUGI != null) { closeUGI(timedProxyUGI); + LOG.info(timedProxyUGI.session.toString() + " Delay Queue Size = " + delayQueue.size()); timedProxyUGI = delayQueue.poll(); } } @@ -240,12 +248,15 @@ private void closeUGI(TimedProxyUGI timedProxyUGI) { try { if (timedProxyUGI.inProgress.get() != 0) { LOG.info(timedProxyUGI.session.toString() + " Skipping close of " + fsMsg); + timedProxyUGI.resetTime(); + delayQueue.offer(timedProxyUGI); return; } - cache.remove(timedProxyUGI.session); - LOG.info(timedProxyUGI.session.toString() + " Closing " + fsMsg + - " (Cache Size = " + cache.size() + ")"); - FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); + if (cache.remove(timedProxyUGI.session) != null) { + LOG.info(timedProxyUGI.session.toString() + " Closing " + fsMsg + + " (Cache Size = " + cache.size() + ")"); + FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); + } } catch (Throwable t) { LOG.warn(timedProxyUGI.session.toString() + " Error closing " + fsMsg); } From 2e3c8dc559ff5deba75a149dee5118710877397f Mon Sep 17 00:00:00 2001 From: lavjain Date: Thu, 5 Jul 2018 15:15:55 -0700 Subject: [PATCH 05/28] Separate out UGICache into a separate class --- .../pxf/service/SegmentTransactionId.java | 60 +++++++ .../hawq/pxf/service/TimedProxyUGI.java | 81 ++++++++++ .../org/apache/hawq/pxf/service/UGICache.java | 123 ++++++++++++++ .../servlet/SecurityServletFilter.java | 153 +----------------- 4 files changed, 272 insertions(+), 145 deletions(-) create mode 100644 pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SegmentTransactionId.java create mode 100644 pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java create mode 100644 pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SegmentTransactionId.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SegmentTransactionId.java new file mode 100644 index 0000000000..32c374a48e --- /dev/null +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SegmentTransactionId.java @@ -0,0 +1,60 @@ +package org.apache.hawq.pxf.service; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +public class SegmentTransactionId { + + private Integer segmentId; + private String transactionId; + private String segmentTransactionId; + + public SegmentTransactionId(Integer segmentId, String transactionId) { + this.segmentId = segmentId; + this.transactionId = transactionId; + this.segmentTransactionId = segmentId + ":" + transactionId; + } + + public Integer getSegmentId() { + return segmentId; + } + + public String getSegmentTransactionId() { + return segmentTransactionId; + } + + @Override + public int hashCode() { + return segmentTransactionId.hashCode(); + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof SegmentTransactionId)) { + return false; + } + SegmentTransactionId that = (SegmentTransactionId) other; + return this.segmentTransactionId.equals(that.segmentTransactionId); + } + + @Override + public String toString() { + return "Session = " + segmentTransactionId; + } +} \ No newline at end of file diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java new file mode 100644 index 0000000000..1ee5bed546 --- /dev/null +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java @@ -0,0 +1,81 @@ +package org.apache.hawq.pxf.service; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.Delayed; +import java.util.concurrent.TimeUnit; + +import org.apache.hadoop.security.UserGroupInformation; + +public class TimedProxyUGI implements Delayed { + + private long startTime; + private UserGroupInformation proxyUGI; + private SegmentTransactionId session; + AtomicInteger inProgress = new AtomicInteger(); + + public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session) { + this.startTime = System.currentTimeMillis(); + this.proxyUGI = proxyUGI; + this.session = session; + inProgress.incrementAndGet(); + } + + public UserGroupInformation getProxyUGI() { + return proxyUGI; + } + + public SegmentTransactionId getSession() { + return session; + } + + public int getCounter() { + return inProgress.get(); + } + + public void incrementCounter() { + inProgress.incrementAndGet(); + } + + public void decrementCounter() { + inProgress.decrementAndGet(); + } + + public void resetTime() { + startTime = System.currentTimeMillis(); + } + + @Override + public long getDelay(TimeUnit unit) { + return unit.convert(getDelayMillis(), TimeUnit.MILLISECONDS); + } + + @Override + public int compareTo(Delayed other) { + TimedProxyUGI that = (TimedProxyUGI) other; + return Long.compare(this.getDelayMillis(), that.getDelayMillis()); + } + + public long getDelayMillis() { + return (startTime + UGICache.UGI_CACHE_EXPIRY) - System.currentTimeMillis(); + } +} + diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java new file mode 100644 index 0000000000..322a4dd977 --- /dev/null +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -0,0 +1,123 @@ +package org.apache.hawq.pxf.service; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import java.io.IOException; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.DelayQueue; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.security.UserGroupInformation; + +public class UGICache { + + private static final Log LOG = LogFactory.getLog(UGICache.class); + private static Map cache = new ConcurrentHashMap<>(); + private static DelayQueue delayQueue = new DelayQueue<>(); + private static Object[] segmentLocks = new Object[100]; + public static long UGI_CACHE_EXPIRY = 15 * 1 * 1000L; // 15 Minutes + + static { + Thread t = new Thread(new Runnable() { + + public void run() { + while (true) { + try { + Thread.sleep(UGI_CACHE_EXPIRY); + TimedProxyUGI timedProxyUGI = delayQueue.poll(); + while (timedProxyUGI != null) { + closeUGI(timedProxyUGI); + LOG.info("Delay Queue Size = " + delayQueue.size()); + timedProxyUGI = delayQueue.poll(); + } + } catch (InterruptedException ie) { + LOG.warn("Terminating reaper thread"); + return; + } + } + } + }); + + t.setName("UGI Reaper Thread"); + t.start(); + for (int i = 0; i < segmentLocks.length; i++) { + segmentLocks[i] = new Object(); + } + } + + public TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) throws IOException { + + Integer segmentId = session.getSegmentId(); + synchronized (segmentLocks[segmentId]) { + TimedProxyUGI timedProxyUGI = cache.get(session); + if (timedProxyUGI == null) { + LOG.info(session.toString() + " Creating proxy user = " + user); + UserGroupInformation proxyUGI = + UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); + timedProxyUGI = new TimedProxyUGI(proxyUGI, session); + delayQueue.offer(timedProxyUGI); + cache.put(session, timedProxyUGI); + } else if (timedProxyUGI.getDelayMillis() < 0) { + closeUGI(timedProxyUGI); + } else { + timedProxyUGI.incrementCounter(); + } + return timedProxyUGI; + } + } + + public void release(TimedProxyUGI timedProxyUGI, Integer fragmentIndex, Integer fragmentCount) { + + Integer segmentId = timedProxyUGI.getSession().getSegmentId(); + synchronized (segmentLocks[segmentId]) { + timedProxyUGI.resetTime(); + timedProxyUGI.decrementCounter(); + if (fragmentIndex != null && fragmentCount.equals(fragmentIndex)) + closeUGI(timedProxyUGI); + } + } + + private static void closeUGI(TimedProxyUGI timedProxyUGI) { + + Integer segmentId = timedProxyUGI.getSession().getSegmentId(); + synchronized (segmentLocks[segmentId]) { + String fsMsg = "FileSystem for proxy user = " + timedProxyUGI.getProxyUGI().getUserName(); + try { + if (timedProxyUGI.inProgress.get() != 0) { + LOG.info(timedProxyUGI.getSession().toString() + " Skipping close of " + fsMsg); + timedProxyUGI.resetTime(); + delayQueue.offer(timedProxyUGI); + return; + } + if (cache.remove(timedProxyUGI.getSession()) != null) { + LOG.info(timedProxyUGI.getSession().toString() + " Closing " + fsMsg + + " (Cache Size = " + cache.size() + ")"); + FileSystem.closeAllForUGI(timedProxyUGI.getProxyUGI()); + } + } catch (Throwable t) { + LOG.warn(timedProxyUGI.getSession().toString() + " Error closing " + fsMsg); + } + } + } + +} \ No newline at end of file diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index b2279f92b2..b9a92dfdf3 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -23,12 +23,6 @@ import java.io.IOException; import java.lang.reflect.UndeclaredThrowableException; import java.security.PrivilegedExceptionAction; -import java.util.Map; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.DelayQueue; -import java.util.concurrent.Delayed; -import java.util.concurrent.TimeUnit; import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -40,9 +34,9 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.security.UserGroupInformation; -import org.apache.hadoop.util.Time; +import org.apache.hawq.pxf.service.SegmentTransactionId; +import org.apache.hawq.pxf.service.TimedProxyUGI; +import org.apache.hawq.pxf.service.UGICache; import org.apache.hawq.pxf.service.utilities.SecureLogin; @@ -60,82 +54,7 @@ public class SecurityServletFilter implements Filter { private static final String FRAGMENT_COUNT_HEADER = "X-GP-FRAGMENT-COUNT"; private static final String MISSING_HEADER_ERROR = "Header %s is missing in the request"; private static final String EMPTY_HEADER_ERROR = "Header %s is empty in the request"; - private static Map cache = new ConcurrentHashMap<>(); - private static DelayQueue delayQueue = new DelayQueue<>(); - private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes - - - private class TimedProxyUGI implements Delayed { - long startTime; - UserGroupInformation proxyUGI; - SegmentTransactionId session; - AtomicInteger inProgress = new AtomicInteger(); - - public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session) { - this.startTime = System.currentTimeMillis(); - this.proxyUGI = proxyUGI; - this.session = session; - inProgress.incrementAndGet(); - } - - public void incrementCounter() { - inProgress.incrementAndGet(); - } - - public void decrementCounter() { - inProgress.decrementAndGet(); - } - - public void resetTime() { - startTime = System.currentTimeMillis(); - } - - @Override - public long getDelay(TimeUnit unit) { - return unit.convert(getDelayMillis(), TimeUnit.MILLISECONDS); - } - - @Override - public int compareTo(Delayed other) { - TimedProxyUGI that = (TimedProxyUGI) other; - return Long.compare(this.getDelayMillis(), that.getDelayMillis()); - } - - private long getDelayMillis() { - return (startTime + UGI_CACHE_EXPIRY) - System.currentTimeMillis(); - } - } - - private class SegmentTransactionId { - String segmentId; - String transactionId; - String segmentTransactionId; - - public SegmentTransactionId(String segmentId, String transactionId) { - this.segmentId = segmentId; - this.transactionId = transactionId; - this.segmentTransactionId = segmentId + ":" + transactionId; - } - - @Override - public int hashCode() { - return segmentTransactionId.hashCode(); - } - - @Override - public boolean equals(Object other) { - if (!(other instanceof SegmentTransactionId)) { - return false; - } - SegmentTransactionId that = (SegmentTransactionId) other; - return this.segmentTransactionId.equals(that.segmentTransactionId); - } - - @Override - public String toString() { - return "Session = " + segmentTransactionId; - } - } + private static UGICache cache = new UGICache(); /** * Initializes the filter. @@ -163,8 +82,8 @@ public void doFilter(final ServletRequest request, final ServletResponse respons // retrieve user header and make sure header is present and is not empty final String user = getHeaderValue(request, USER_HEADER); - String segmentId = getHeaderValue(request, SEGMENT_ID_HEADER); String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER); + Integer segmentId = getHeaderValueInt(request, SEGMENT_ID_HEADER, true); Integer fragmentCount = getHeaderValueInt(request, FRAGMENT_COUNT_HEADER, false); Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); @@ -188,9 +107,9 @@ 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 - TimedProxyUGI timedProxyUGI = getTimedProxyUGI(user, session); + TimedProxyUGI timedProxyUGI = cache.getTimedProxyUGI(user, session); try { - timedProxyUGI.proxyUGI.doAs(action); + timedProxyUGI.getProxyUGI().doAs(action); } catch (UndeclaredThrowableException ute) { // unwrap the real exception thrown by the action throw new ServletException(ute.getCause()); @@ -198,7 +117,7 @@ public Boolean run() throws IOException, ServletException { throw new ServletException(ie); } finally { - release(timedProxyUGI, fragmentIndex, fragmentCount); + cache.release(timedProxyUGI, fragmentIndex, fragmentCount); } } else { // no user impersonation is configured @@ -206,62 +125,6 @@ public Boolean run() throws IOException, ServletException { } } - private TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) throws IOException { - synchronized (session.segmentTransactionId.intern()) { - TimedProxyUGI timedProxyUGI = cache.get(session); - if (timedProxyUGI == null || timedProxyUGI.getDelayMillis() < 0) { - cleanup(); - LOG.info(session.toString() + " Creating proxy user = " + user); - UserGroupInformation proxyUGI = - UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); - timedProxyUGI = new TimedProxyUGI(proxyUGI, session); - delayQueue.offer(timedProxyUGI); - cache.put(session, timedProxyUGI); - } else { - timedProxyUGI.incrementCounter(); - } - return timedProxyUGI; - } - } - - private void release(TimedProxyUGI timedProxyUGI, Integer fragmentIndex, Integer fragmentCount) { - synchronized (timedProxyUGI.session.segmentTransactionId.intern()) { - timedProxyUGI.resetTime(); - timedProxyUGI.decrementCounter(); - if (fragmentIndex != null && fragmentCount.equals(fragmentIndex)) - closeUGI(timedProxyUGI); - } - } - - private void cleanup() { - TimedProxyUGI timedProxyUGI = delayQueue.poll(); - while (timedProxyUGI != null) { - closeUGI(timedProxyUGI); - LOG.info(timedProxyUGI.session.toString() + " Delay Queue Size = " + delayQueue.size()); - timedProxyUGI = delayQueue.poll(); - } - } - - private void closeUGI(TimedProxyUGI timedProxyUGI) { - synchronized (timedProxyUGI.session.segmentTransactionId.intern()) { - String fsMsg = "FileSystem for proxy user = " + timedProxyUGI.proxyUGI.getUserName(); - try { - if (timedProxyUGI.inProgress.get() != 0) { - LOG.info(timedProxyUGI.session.toString() + " Skipping close of " + fsMsg); - timedProxyUGI.resetTime(); - delayQueue.offer(timedProxyUGI); - return; - } - if (cache.remove(timedProxyUGI.session) != null) { - LOG.info(timedProxyUGI.session.toString() + " Closing " + fsMsg + - " (Cache Size = " + cache.size() + ")"); - FileSystem.closeAllForUGI(timedProxyUGI.proxyUGI); - } - } catch (Throwable t) { - LOG.warn(timedProxyUGI.session.toString() + " Error closing " + fsMsg); - } - } - } private Integer getHeaderValueInt(ServletRequest request, String headerKey, boolean required) throws IllegalArgumentException { From ce850e636bdbd9ba8b560c82388d9555c92dfe4c Mon Sep 17 00:00:00 2001 From: Lav Jain Date: Thu, 5 Jul 2018 18:10:08 -0700 Subject: [PATCH 06/28] WIP: Handle any edge cases in the previous design --- .../hawq/pxf/service/TimedProxyUGI.java | 16 ++- .../org/apache/hawq/pxf/service/UGICache.java | 112 +++++++++--------- .../servlet/SecurityServletFilter.java | 3 +- 3 files changed, 75 insertions(+), 56 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java index 1ee5bed546..6364c9dbcf 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java @@ -27,16 +27,16 @@ public class TimedProxyUGI implements Delayed { - private long startTime; + private volatile long startTime; private UserGroupInformation proxyUGI; private SegmentTransactionId session; + private boolean cleaned = false; AtomicInteger inProgress = new AtomicInteger(); public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session) { this.startTime = System.currentTimeMillis(); this.proxyUGI = proxyUGI; this.session = session; - inProgress.incrementAndGet(); } public UserGroupInformation getProxyUGI() { @@ -47,6 +47,14 @@ public SegmentTransactionId getSession() { return session; } + public boolean isCleaned() { + return cleaned; + } + + public void setCleaned() { + cleaned = true; + } + public int getCounter() { return inProgress.get(); } @@ -63,6 +71,10 @@ public void resetTime() { startTime = System.currentTimeMillis(); } + public void setExpired() { + startTime = -1L; + } + @Override public long getDelay(TimeUnit unit) { return unit.convert(getDelayMillis(), TimeUnit.MILLISECONDS); diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 322a4dd977..f31645b4d1 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -33,91 +33,97 @@ public class UGICache { private static final Log LOG = LogFactory.getLog(UGICache.class); private static Map cache = new ConcurrentHashMap<>(); - private static DelayQueue delayQueue = new DelayQueue<>(); - private static Object[] segmentLocks = new Object[100]; + //private static DelayQueue delayQueue = new DelayQueue<>(); + private static DelayQueue[] delayQueues = new DelayQueue<>[64]; public static long UGI_CACHE_EXPIRY = 15 * 1 * 1000L; // 15 Minutes - static { - Thread t = new Thread(new Runnable() { - - public void run() { - while (true) { - try { - Thread.sleep(UGI_CACHE_EXPIRY); - TimedProxyUGI timedProxyUGI = delayQueue.poll(); - while (timedProxyUGI != null) { - closeUGI(timedProxyUGI); - LOG.info("Delay Queue Size = " + delayQueue.size()); - timedProxyUGI = delayQueue.poll(); - } - } catch (InterruptedException ie) { - LOG.warn("Terminating reaper thread"); - return; - } - } - } - }); - - t.setName("UGI Reaper Thread"); - t.start(); - for (int i = 0; i < segmentLocks.length; i++) { - segmentLocks[i] = new Object(); + public UGICache() { + for (int i = 0; i < delayQueues.length; i++) { + delayQueues[i] = new DelayQueue<>(); } } public TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) throws IOException { Integer segmentId = session.getSegmentId(); - synchronized (segmentLocks[segmentId]) { + synchronized (delayQueues[segmentId]) { + // use the opportunity to cleanup any expired entries + cleanup(segmentId); TimedProxyUGI timedProxyUGI = cache.get(session); if (timedProxyUGI == null) { LOG.info(session.toString() + " Creating proxy user = " + user); UserGroupInformation proxyUGI = UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); timedProxyUGI = new TimedProxyUGI(proxyUGI, session); - delayQueue.offer(timedProxyUGI); + delayQueues[segmentId].offer(timedProxyUGI); cache.put(session, timedProxyUGI); - } else if (timedProxyUGI.getDelayMillis() < 0) { - closeUGI(timedProxyUGI); - } else { - timedProxyUGI.incrementCounter(); } + timedProxyUGI.incrementCounter(); return timedProxyUGI; } } - public void release(TimedProxyUGI timedProxyUGI, Integer fragmentIndex, Integer fragmentCount) { + private cleanup(Integer segmentId) { + // poll segment expiration queue for all expired entries and clean them if possible + TimedProxyUGI ugi = null; + while ((ugi = delayQueues[segmentId].poll()) != null) { + // place it back in the queue if still in use and was not closed + if (!closeUGI(ugi)) { + delayQueues[segmentId].offer(ugi); + } + LOG.info("Delay Queue Size for segment " + + segmentId + " = " + delayQueues[segmentId].size()); + } + } + + public void release(TimedProxyUGI timedProxyUGI, boolean forceClean) { Integer segmentId = timedProxyUGI.getSession().getSegmentId(); - synchronized (segmentLocks[segmentId]) { - timedProxyUGI.resetTime(); - timedProxyUGI.decrementCounter(); - if (fragmentIndex != null && fragmentCount.equals(fragmentIndex)) + + timedProxyUGI.resetTime(); + timedProxyUGI.decrementCounter(); + + if (forceClean) { + synchronized (delayQueues[segmentId]) { + timedProxyUGI.setExpired(); closeUGI(timedProxyUGI); + } } } - private static void closeUGI(TimedProxyUGI timedProxyUGI) { + private static boolean closeUGI(TimedProxyUGI expiredProxyUGI) { - Integer segmentId = timedProxyUGI.getSession().getSegmentId(); - synchronized (segmentLocks[segmentId]) { - String fsMsg = "FileSystem for proxy user = " + timedProxyUGI.getProxyUGI().getUserName(); + SegmentTransactionId session = expiredProxyUGI.getSession(); + Integer segmentId = session.getSegmentId(); + //synchronized (delayQueues[segmentId]) { + String fsMsg = "FileSystem for proxy user = " + expiredProxyUGI.getProxyUGI().getUserName(); try { - if (timedProxyUGI.inProgress.get() != 0) { - LOG.info(timedProxyUGI.getSession().toString() + " Skipping close of " + fsMsg); - timedProxyUGI.resetTime(); - delayQueue.offer(timedProxyUGI); - return; + // The UGI object is still being used by another thread + if (expiredProxyUGI.inProgress.get() != 0) { + LOG.info(session.toString() + " Skipping close of " + fsMsg); + expiredProxyUGI.resetTime(); // update time so that it doesn't expire until release updates time again + return false; } - if (cache.remove(timedProxyUGI.getSession()) != null) { - LOG.info(timedProxyUGI.getSession().toString() + " Closing " + fsMsg + + // expired UGI can be cleaned, as it is not used + // determine if it can be removed from cache also + TimedProxyUGI cachedUGI = cache.get(session); + + if (expiredProxyUGI == cachedUGI) { + // remove it from cache, as cache now has expired entry which is not in progress + cache.remove(session); + } + + if (!expiredProxyUGI.isCleaned()) { + LOG.info(session.toString() + " Closing " + fsMsg + " (Cache Size = " + cache.size() + ")"); - FileSystem.closeAllForUGI(timedProxyUGI.getProxyUGI()); + FileSystem.closeAllForUGI(expiredProxyUGI.getProxyUGI()); + expiredProxyUGI.setCleaned(); } + } catch (Throwable t) { - LOG.warn(timedProxyUGI.getSession().toString() + " Error closing " + fsMsg); + LOG.warn(session.toString() + " Error closing " + fsMsg); } - } + //} } -} \ No newline at end of file +} diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index b9a92dfdf3..98a3c7f29d 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -117,7 +117,8 @@ public Boolean run() throws IOException, ServletException { throw new ServletException(ie); } finally { - cache.release(timedProxyUGI, fragmentIndex, fragmentCount); + boolean forceClean = (fragmentIndex != null && fragmentCount.equals(fragmentIndex)); + cache.release(timedProxyUGI, forceClean); } } else { // no user impersonation is configured From 0356653102715d6d1a39113c8f1de2e787b8f52e Mon Sep 17 00:00:00 2001 From: lavjain Date: Fri, 6 Jul 2018 19:19:30 -0700 Subject: [PATCH 07/28] Finalize the code and add comments --- .../org/apache/hawq/pxf/service/UGICache.java | 87 ++++++++++--------- .../servlet/SecurityServletFilter.java | 5 +- 2 files changed, 50 insertions(+), 42 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index f31645b4d1..d21723a028 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -33,8 +33,9 @@ public class UGICache { private static final Log LOG = LogFactory.getLog(UGICache.class); private static Map cache = new ConcurrentHashMap<>(); - //private static DelayQueue delayQueue = new DelayQueue<>(); - private static DelayQueue[] delayQueues = new DelayQueue<>[64]; + @SuppressWarnings("unchecked") + // There is a separate DelayQueue for each segment (also being used for locking) + private static DelayQueue[] delayQueues = (DelayQueue[])new DelayQueue[64]; public static long UGI_CACHE_EXPIRY = 15 * 1 * 1000L; // 15 Minutes public UGICache() { @@ -43,11 +44,13 @@ public UGICache() { } } - public TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) throws IOException { + // Create new proxy UGI if not found in cache and increment reference count + public TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) + throws IOException { Integer segmentId = session.getSegmentId(); synchronized (delayQueues[segmentId]) { - // use the opportunity to cleanup any expired entries + // Use the opportunity to cleanup any expired entries cleanup(segmentId); TimedProxyUGI timedProxyUGI = cache.get(session); if (timedProxyUGI == null) { @@ -63,26 +66,26 @@ public TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) } } - private cleanup(Integer segmentId) { - // poll segment expiration queue for all expired entries and clean them if possible + // Poll segment expiration queue for all expired entries and clean them if possible + private void cleanup(Integer segmentId) { + TimedProxyUGI ugi = null; while ((ugi = delayQueues[segmentId].poll()) != null) { - // place it back in the queue if still in use and was not closed + // Place it back in the queue if still in use and was not closed if (!closeUGI(ugi)) { delayQueues[segmentId].offer(ugi); } - LOG.info("Delay Queue Size for segment " + + LOG.debug("Delay Queue Size for segment " + segmentId + " = " + delayQueues[segmentId].size()); } } - + + // Decrement reference count and optional cleanup public void release(TimedProxyUGI timedProxyUGI, boolean forceClean) { Integer segmentId = timedProxyUGI.getSession().getSegmentId(); - timedProxyUGI.resetTime(); timedProxyUGI.decrementCounter(); - if (forceClean) { synchronized (delayQueues[segmentId]) { timedProxyUGI.setExpired(); @@ -91,39 +94,43 @@ public void release(TimedProxyUGI timedProxyUGI, boolean forceClean) { } } - private static boolean closeUGI(TimedProxyUGI expiredProxyUGI) { + // There is no need to synchronize this method because it should be called + // from within a synchronized block + private boolean closeUGI(TimedProxyUGI expiredProxyUGI) { SegmentTransactionId session = expiredProxyUGI.getSession(); Integer segmentId = session.getSegmentId(); - //synchronized (delayQueues[segmentId]) { - String fsMsg = "FileSystem for proxy user = " + expiredProxyUGI.getProxyUGI().getUserName(); - try { - // The UGI object is still being used by another thread - if (expiredProxyUGI.inProgress.get() != 0) { - LOG.info(session.toString() + " Skipping close of " + fsMsg); - expiredProxyUGI.resetTime(); // update time so that it doesn't expire until release updates time again - return false; - } - // expired UGI can be cleaned, as it is not used - // determine if it can be removed from cache also - TimedProxyUGI cachedUGI = cache.get(session); - - if (expiredProxyUGI == cachedUGI) { - // remove it from cache, as cache now has expired entry which is not in progress - cache.remove(session); - } - - if (!expiredProxyUGI.isCleaned()) { - LOG.info(session.toString() + " Closing " + fsMsg + - " (Cache Size = " + cache.size() + ")"); - FileSystem.closeAllForUGI(expiredProxyUGI.getProxyUGI()); - expiredProxyUGI.setCleaned(); - } - - } catch (Throwable t) { - LOG.warn(session.toString() + " Error closing " + fsMsg); + String fsMsg = "FileSystem for proxy user = " + expiredProxyUGI.getProxyUGI().getUserName(); + try { + // The UGI object is still being used by another thread + if (expiredProxyUGI.inProgress.get() != 0) { + LOG.info(session.toString() + " Skipping close of " + fsMsg); + // Update time so that it doesn't expire until release updates time again + expiredProxyUGI.resetTime(); + return false; + } + + // Expired UGI object can be cleaned since it is not used + // Determine if it can be removed from cache also + TimedProxyUGI cachedUGI = cache.get(session); + if (expiredProxyUGI == cachedUGI) { + // Remove it from cache, as cache now has an + // expired entry which is not in progress + cache.remove(session); } - //} + + // Optimization to call close only if it has not been called for that UGI + if (!expiredProxyUGI.isCleaned()) { + LOG.info(session.toString() + " Closing " + fsMsg + + " (Cache Size = " + cache.size() + ")"); + FileSystem.closeAllForUGI(expiredProxyUGI.getProxyUGI()); + expiredProxyUGI.setCleaned(); + } + + } catch (Throwable t) { + LOG.warn(session.toString() + " Error closing " + fsMsg); + } + return true; } } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 98a3c7f29d..10a3362bb0 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -88,10 +88,10 @@ public void doFilter(final ServletRequest request, final ServletResponse respons Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); SegmentTransactionId session = new SegmentTransactionId(segmentId, transactionId); - if (fragmentCount != null) { + if (LOG.isDebugEnabled() && fragmentCount != null) { StringBuilder sb = new StringBuilder(session.toString()); sb.append(" Fragment = ").append(fragmentIndex).append(" of ").append(fragmentCount); - LOG.info(sb.toString()); + LOG.debug(sb.toString()); } // TODO refresh Kerberos token when security is enabled @@ -117,6 +117,7 @@ public Boolean run() throws IOException, ServletException { throw new ServletException(ie); } finally { + // Optimization to cleanup the cache if it is the last fragment boolean forceClean = (fragmentIndex != null && fragmentCount.equals(fragmentIndex)); cache.release(timedProxyUGI, forceClean); } From da59fbac785d20816c5f298808e699b2bab302ef Mon Sep 17 00:00:00 2001 From: Lav Jain Date: Tue, 10 Jul 2018 14:58:38 -0700 Subject: [PATCH 08/28] Add unit tests for UGICache class - Renamed TimedProxyUGI to UGICacheEntry, SegmentTransactionId to SessionId - SessionId now contains the user name - Use UGIProvider to create and destroy UGIs [#158018994] Co-authored-by: Lav Jain Co-authored-by: Ben Christel --- ...gmentTransactionId.java => SessionId.java} | 32 +-- .../org/apache/hawq/pxf/service/UGICache.java | 49 +++-- ...{TimedProxyUGI.java => UGICacheEntry.java} | 17 +- .../apache/hawq/pxf/service/UGIProvider.java | 37 ++++ .../servlet/SecurityServletFilter.java | 11 +- .../apache/hawq/pxf/service/UGICacheTest.java | 190 ++++++++++++++++++ 6 files changed, 285 insertions(+), 51 deletions(-) rename pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/{SegmentTransactionId.java => SessionId.java} (63%) rename pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/{TimedProxyUGI.java => UGICacheEntry.java} (81%) create mode 100644 pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java create mode 100644 pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SegmentTransactionId.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java similarity index 63% rename from pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SegmentTransactionId.java rename to pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java index 32c374a48e..f827043e96 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SegmentTransactionId.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java @@ -19,42 +19,42 @@ * under the License. */ -public class SegmentTransactionId { +public class SessionId { + private final String user; private Integer segmentId; - private String transactionId; - private String segmentTransactionId; + private String sessionId; - public SegmentTransactionId(Integer segmentId, String transactionId) { + public SessionId(Integer segmentId, String transactionId, String gpdbUser) { this.segmentId = segmentId; - this.transactionId = transactionId; - this.segmentTransactionId = segmentId + ":" + transactionId; + this.user = gpdbUser; + this.sessionId = segmentId + ":" + transactionId + ":" + gpdbUser; } public Integer getSegmentId() { return segmentId; } - public String getSegmentTransactionId() { - return segmentTransactionId; - } - @Override public int hashCode() { - return segmentTransactionId.hashCode(); + return sessionId.hashCode(); } @Override public boolean equals(Object other) { - if (!(other instanceof SegmentTransactionId)) { + if (!(other instanceof SessionId)) { return false; } - SegmentTransactionId that = (SegmentTransactionId) other; - return this.segmentTransactionId.equals(that.segmentTransactionId); + SessionId that = (SessionId) other; + return this.sessionId.equals(that.sessionId); } @Override public String toString() { - return "Session = " + segmentTransactionId; + return "Session = " + sessionId; + } + + public String getUser() { + return user; } -} \ No newline at end of file +} diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index d21723a028..8980578c09 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -32,32 +32,37 @@ public class UGICache { private static final Log LOG = LogFactory.getLog(UGICache.class); - private static Map cache = new ConcurrentHashMap<>(); + private Map cache = new ConcurrentHashMap<>(); @SuppressWarnings("unchecked") // There is a separate DelayQueue for each segment (also being used for locking) - private static DelayQueue[] delayQueues = (DelayQueue[])new DelayQueue[64]; - public static long UGI_CACHE_EXPIRY = 15 * 1 * 1000L; // 15 Minutes + private DelayQueue[] delayQueues = (DelayQueue[])new DelayQueue[64]; + private final UGIProvider ugiProvider; - public UGICache() { + public UGICache(UGIProvider provider) { + this.ugiProvider = provider; for (int i = 0; i < delayQueues.length; i++) { delayQueues[i] = new DelayQueue<>(); } } + public UGICache() { + this(new UGIProvider()); + } + // Create new proxy UGI if not found in cache and increment reference count - public TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) + public UGICacheEntry getTimedProxyUGI(SessionId session) throws IOException { Integer segmentId = session.getSegmentId(); + String user = session.getUser(); synchronized (delayQueues[segmentId]) { // Use the opportunity to cleanup any expired entries cleanup(segmentId); - TimedProxyUGI timedProxyUGI = cache.get(session); + UGICacheEntry timedProxyUGI = cache.get(session); if (timedProxyUGI == null) { LOG.info(session.toString() + " Creating proxy user = " + user); - UserGroupInformation proxyUGI = - UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser()); - timedProxyUGI = new TimedProxyUGI(proxyUGI, session); + UserGroupInformation proxyUGI = ugiProvider.createProxyUGI(user); + timedProxyUGI = new UGICacheEntry(proxyUGI, session); delayQueues[segmentId].offer(timedProxyUGI); cache.put(session, timedProxyUGI); } @@ -66,10 +71,11 @@ public TimedProxyUGI getTimedProxyUGI(String user, SegmentTransactionId session) } } - // Poll segment expiration queue for all expired entries and clean them if possible + // Poll segment expiration queue for all expired entries + // and clean them if possible private void cleanup(Integer segmentId) { - TimedProxyUGI ugi = null; + UGICacheEntry ugi = null; while ((ugi = delayQueues[segmentId].poll()) != null) { // Place it back in the queue if still in use and was not closed if (!closeUGI(ugi)) { @@ -81,7 +87,7 @@ private void cleanup(Integer segmentId) { } // Decrement reference count and optional cleanup - public void release(TimedProxyUGI timedProxyUGI, boolean forceClean) { + public void release(UGICacheEntry timedProxyUGI, boolean forceClean) { Integer segmentId = timedProxyUGI.getSession().getSegmentId(); timedProxyUGI.resetTime(); @@ -96,34 +102,35 @@ public void release(TimedProxyUGI timedProxyUGI, boolean forceClean) { // There is no need to synchronize this method because it should be called // from within a synchronized block - private boolean closeUGI(TimedProxyUGI expiredProxyUGI) { + private boolean closeUGI(UGICacheEntry expiredProxyUGI) { - SegmentTransactionId session = expiredProxyUGI.getSession(); - Integer segmentId = session.getSegmentId(); - String fsMsg = "FileSystem for proxy user = " + expiredProxyUGI.getProxyUGI().getUserName(); + SessionId session = expiredProxyUGI.getSession(); + String fsMsg = "FileSystem for proxy user = " + expiredProxyUGI.getUGI().getUserName(); try { // The UGI object is still being used by another thread - if (expiredProxyUGI.inProgress.get() != 0) { + if (expiredProxyUGI.getCounter() != 0) { LOG.info(session.toString() + " Skipping close of " + fsMsg); - // Update time so that it doesn't expire until release updates time again + // Reset time so that it doesn't expire until release + // updates the time again expiredProxyUGI.resetTime(); return false; } // Expired UGI object can be cleaned since it is not used // Determine if it can be removed from cache also - TimedProxyUGI cachedUGI = cache.get(session); + UGICacheEntry cachedUGI = cache.get(session); if (expiredProxyUGI == cachedUGI) { // Remove it from cache, as cache now has an // expired entry which is not in progress cache.remove(session); } - // Optimization to call close only if it has not been called for that UGI + // Optimization to call close only if it has not been + // called for that UGI if (!expiredProxyUGI.isCleaned()) { LOG.info(session.toString() + " Closing " + fsMsg + " (Cache Size = " + cache.size() + ")"); - FileSystem.closeAllForUGI(expiredProxyUGI.getProxyUGI()); + ugiProvider.destroy(expiredProxyUGI.getUGI()); expiredProxyUGI.setCleaned(); } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java similarity index 81% rename from pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java rename to pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java index 6364c9dbcf..7cab5ff57f 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/TimedProxyUGI.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java @@ -25,25 +25,26 @@ import org.apache.hadoop.security.UserGroupInformation; -public class TimedProxyUGI implements Delayed { +public class UGICacheEntry implements Delayed { private volatile long startTime; private UserGroupInformation proxyUGI; - private SegmentTransactionId session; + private SessionId session; private boolean cleaned = false; - AtomicInteger inProgress = new AtomicInteger(); + private AtomicInteger inProgress = new AtomicInteger(); + private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes - public TimedProxyUGI(UserGroupInformation proxyUGI, SegmentTransactionId session) { + public UGICacheEntry(UserGroupInformation proxyUGI, SessionId session) { this.startTime = System.currentTimeMillis(); this.proxyUGI = proxyUGI; this.session = session; } - public UserGroupInformation getProxyUGI() { + public UserGroupInformation getUGI() { return proxyUGI; } - public SegmentTransactionId getSession() { + public SessionId getSession() { return session; } @@ -82,12 +83,12 @@ public long getDelay(TimeUnit unit) { @Override public int compareTo(Delayed other) { - TimedProxyUGI that = (TimedProxyUGI) other; + UGICacheEntry that = (UGICacheEntry) other; return Long.compare(this.getDelayMillis(), that.getDelayMillis()); } public long getDelayMillis() { - return (startTime + UGICache.UGI_CACHE_EXPIRY) - System.currentTimeMillis(); + return (startTime + UGI_CACHE_EXPIRY) - System.currentTimeMillis(); } } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java new file mode 100644 index 0000000000..69a007c626 --- /dev/null +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java @@ -0,0 +1,37 @@ +package org.apache.hawq.pxf.service; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.security.UserGroupInformation; + +import java.io.IOException; + +public class UGIProvider { + + public UserGroupInformation createProxyUGI(String effectiveUser) throws IOException { + return UserGroupInformation.createProxyUser( + effectiveUser, UserGroupInformation.getLoginUser()); + } + + public void destroy(UserGroupInformation ugi) throws IOException { + FileSystem.closeAllForUGI(ugi); + } +} diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 10a3362bb0..1426668c8d 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -34,8 +34,8 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.hawq.pxf.service.SegmentTransactionId; -import org.apache.hawq.pxf.service.TimedProxyUGI; +import org.apache.hawq.pxf.service.SessionId; +import org.apache.hawq.pxf.service.UGICacheEntry; import org.apache.hawq.pxf.service.UGICache; import org.apache.hawq.pxf.service.utilities.SecureLogin; @@ -47,7 +47,6 @@ public class SecurityServletFilter implements Filter { private static final Log LOG = LogFactory.getLog(SecurityServletFilter.class); private static final String USER_HEADER = "X-GP-USER"; - private static final String SEGMENT_COUNT_HEADER = "X-GP-SEGMENT-COUNT"; private static final String SEGMENT_ID_HEADER = "X-GP-SEGMENT-ID"; private static final String TRANSACTION_ID_HEADER = "X-GP-XID"; private static final String FRAGMENT_INDEX_HEADER = "X-GP-FRAGMENT-INDEX"; @@ -87,7 +86,7 @@ public void doFilter(final ServletRequest request, final ServletResponse respons Integer fragmentCount = getHeaderValueInt(request, FRAGMENT_COUNT_HEADER, false); Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); - SegmentTransactionId session = new SegmentTransactionId(segmentId, transactionId); + SessionId session = new SessionId(segmentId, transactionId, user); if (LOG.isDebugEnabled() && fragmentCount != null) { StringBuilder sb = new StringBuilder(session.toString()); sb.append(" Fragment = ").append(fragmentIndex).append(" of ").append(fragmentCount); @@ -107,9 +106,9 @@ 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 - TimedProxyUGI timedProxyUGI = cache.getTimedProxyUGI(user, session); + UGICacheEntry timedProxyUGI = cache.getTimedProxyUGI(session); try { - timedProxyUGI.getProxyUGI().doAs(action); + timedProxyUGI.getUGI().doAs(action); } catch (UndeclaredThrowableException ute) { // unwrap the real exception thrown by the action throw new ServletException(ute.getCause()); diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java new file mode 100644 index 0000000000..c0e3881325 --- /dev/null +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -0,0 +1,190 @@ +package org.apache.hawq.pxf.service; + +import org.apache.hadoop.security.UserGroupInformation; +import org.junit.Before; +import org.junit.Test; +import org.mockito.stubbing.Answer; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.*; +import static org.powermock.api.mockito.PowerMockito.when; + +public class UGICacheTest { + private UGIProvider provider = null; + private SessionId session = null; + private UGICache cache = null; + + @Before + public void setUp() throws Exception { + provider = mock(UGIProvider.class); + + when(provider.createProxyUGI(any(String.class))).thenAnswer((Answer) invocation -> mock(UserGroupInformation.class)); + + session = new SessionId(0, "txn-id", "the-user"); + + cache = new UGICache(provider); + } + + @Test + public void getUGIFromEmptyCache() throws Exception { + UGICacheEntry entry = cache.getTimedProxyUGI(session); + assertNotNull(entry.getUGI()); + verify(provider).createProxyUGI("the-user"); + } + + @Test + public void getSameUGITwiceUsesCache() throws Exception { + UGICacheEntry entry1 = cache.getTimedProxyUGI(session); + UGICacheEntry entry2 = cache.getTimedProxyUGI(session); + assertEquals(entry1, entry2); + assertNotNull(entry1.getUGI()); + verify(provider, times(1)).createProxyUGI("the-user"); + } + + @Test + public void getTwoUGIsWithDifferentSessionsForSameUser() throws Exception { + SessionId otherSession = new SessionId(0, "txn-id-2", "the-user"); + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(otherSession); + assertNotEquals(proxyUGI1, proxyUGI2); + verify(provider, times(2)).createProxyUGI("the-user"); + // TODO: this seems weird. We're creating two UGIs with the same params, + // even though we have two different sessions. Why? + } + + @Test + public void getTwoUGIsWithDifferentUsers() throws Exception { + SessionId otherSession = new SessionId(0, "txn-id", "different-user"); + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(otherSession); + assertNotEquals(proxyUGI1, proxyUGI2); + verify(provider, times(1)).createProxyUGI("the-user"); + verify(provider, times(1)).createProxyUGI("different-user"); + } + + @Test + public void getTwoUGIsWithDifferentUsersCachesBoth() throws Exception { + SessionId otherSession = new SessionId(0, "txn-id", "different-user"); + UGICacheEntry proxyUGI1a = cache.getTimedProxyUGI(session); + UGICacheEntry proxyUGI1b = cache.getTimedProxyUGI(session); + UGICacheEntry proxyUGI2a = cache.getTimedProxyUGI(otherSession); + UGICacheEntry proxyUGI2b = cache.getTimedProxyUGI(otherSession); + assertEquals(proxyUGI1a, proxyUGI1b); + assertEquals(proxyUGI2a, proxyUGI2b); + assertNotEquals(proxyUGI1a, proxyUGI2a); + verify(provider, times(1)).createProxyUGI("the-user"); + verify(provider, times(1)).createProxyUGI("different-user"); + } + + @Test + public void getUGIWhenRequestedUserDoesNotExist() throws Exception { + // what does UserGroupInformation.createProxyUser() do in this scenario? + // how about getLoginUser()? + } + + @Test + public void releaseWithoutForceClean() throws Exception { + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + + cache.release(proxyUGI1, false); + // UGI wasn't cleaned up, so we can still get it + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + assertEquals(proxyUGI1, proxyUGI2); + verify(provider, times(1)).createProxyUGI("the-user"); + } + + @Test + public void releaseWithForceClean() throws Exception { + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + + cache.release(proxyUGI1, true); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + assertNotEquals(proxyUGI1, proxyUGI2); + verify(provider, times(2)).createProxyUGI("the-user"); + } + + @Test + public void releaseAndReacquireDoesNotFreeResources() throws Exception { + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + + cache.release(proxyUGI1, false); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + proxyUGI2.setExpired(); + UGICacheEntry proxyUGI3 = cache.getTimedProxyUGI(session); + // this does not clean up any UGIs because our proxyUGI is still in use. + assertEquals(proxyUGI3, proxyUGI2); + verify(provider, times(1)).createProxyUGI("the-user"); + verify(provider, never()).destroy(any(UserGroupInformation.class)); + } + + @Test + public void releaseAndAcquireAfterTimeoutFreesResources() throws Exception { + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + + cache.release(proxyUGI1, false); + proxyUGI1.setExpired(); + verify(provider, never()).destroy(any(UserGroupInformation.class)); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + verify(provider).destroy(proxyUGI1.getUGI()); + assertNotEquals(proxyUGI2, proxyUGI1); + assertNotEquals(proxyUGI2.getUGI(), proxyUGI1.getUGI()); + verify(provider, never()).destroy(proxyUGI2.getUGI()); + // this does not clean up any UGIs because our proxyUGI is still in use. + assertNotEquals(proxyUGI2, proxyUGI1); + verify(provider, times(2)).createProxyUGI("the-user"); + } + + @Test + public void releaseAnExpiredUGIResetsTheTimer() throws Exception { + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + proxyUGI1.setExpired(); + cache.release(proxyUGI1, false); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + assertEquals(proxyUGI2, proxyUGI1); + verify(provider, never()).destroy(any(UserGroupInformation.class)); + } + + @Test + public void releaseDoesNotFreeResourcesIfUGIIsUsedElsewhere() throws Exception { + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + + cache.release(proxyUGI1, true); + // UGI was not cleaned up because proxyUGI2 is referencing it + UGICacheEntry proxyUGI3 = cache.getTimedProxyUGI(session); + assertEquals(proxyUGI1, proxyUGI2); + assertEquals(proxyUGI1, proxyUGI3); + verify(provider, times(1)).createProxyUGI("the-user"); + } + + @Test + public void releasingAllReferencesFreesResources() throws Exception { + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + + cache.release(proxyUGI1, true); + cache.release(proxyUGI2, true); + // at this point, the initial UGI has been freed. Calling + // getTimedProxyUGI again creates a new UGI. + UGICacheEntry proxyUGI3 = cache.getTimedProxyUGI(session); + assertEquals(proxyUGI1, proxyUGI2); + assertNotEquals(proxyUGI1, proxyUGI3); + verify(provider, times(2)).createProxyUGI("the-user"); + } + + @Test + public void releaseAnEntryNotInTheCache() throws Exception { + // this could happen if some caller of the cache + // retains a reference to a cache entry after releasing it. + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + + cache.release(proxyUGI1, true); + cache.release(proxyUGI1, true); + UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + assertNotEquals(proxyUGI1, proxyUGI2); + verify(provider, times(2)).createProxyUGI("the-user"); + } +} From c05c389f90ed29e3605deb53f7711dcbc43d06a7 Mon Sep 17 00:00:00 2001 From: Alex Denissov Date: Tue, 10 Jul 2018 15:55:19 -0700 Subject: [PATCH 09/28] Change DelayQueue array implementation to use HashMap to handle segment ids greater than 64 Co-authored-by: Lav Jain Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 39 ++++++++++++------- .../apache/hawq/pxf/service/UGICacheTest.java | 7 ++++ 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 8980578c09..a61bd99247 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -20,14 +20,13 @@ */ import java.io.IOException; +import java.util.HashMap; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.DelayQueue; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.hadoop.fs.FileSystem; -import org.apache.hadoop.security.UserGroupInformation; public class UGICache { @@ -35,35 +34,46 @@ public class UGICache { private Map cache = new ConcurrentHashMap<>(); @SuppressWarnings("unchecked") // There is a separate DelayQueue for each segment (also being used for locking) - private DelayQueue[] delayQueues = (DelayQueue[])new DelayQueue[64]; + private final Map> queueMap = new HashMap<>(); private final UGIProvider ugiProvider; public UGICache(UGIProvider provider) { this.ugiProvider = provider; - for (int i = 0; i < delayQueues.length; i++) { - delayQueues[i] = new DelayQueue<>(); - } } public UGICache() { this(new UGIProvider()); } + private DelayQueue getDelayQueue(Integer segmentId) { + DelayQueue queue = queueMap.get(segmentId); + if (queue == null) { + synchronized (queueMap) { + queue = queueMap.get(segmentId); + if (queue == null) { + queue = new DelayQueue<>(); + queueMap.put(segmentId, queue); + } + } + } + return queue; + } + // Create new proxy UGI if not found in cache and increment reference count public UGICacheEntry getTimedProxyUGI(SessionId session) throws IOException { Integer segmentId = session.getSegmentId(); String user = session.getUser(); - synchronized (delayQueues[segmentId]) { + DelayQueue delayQueue = getDelayQueue(segmentId); + synchronized (delayQueue) { // Use the opportunity to cleanup any expired entries cleanup(segmentId); UGICacheEntry timedProxyUGI = cache.get(session); if (timedProxyUGI == null) { LOG.info(session.toString() + " Creating proxy user = " + user); - UserGroupInformation proxyUGI = ugiProvider.createProxyUGI(user); - timedProxyUGI = new UGICacheEntry(proxyUGI, session); - delayQueues[segmentId].offer(timedProxyUGI); + timedProxyUGI = new UGICacheEntry(ugiProvider.createProxyUGI(user), session); + delayQueue.offer(timedProxyUGI); cache.put(session, timedProxyUGI); } timedProxyUGI.incrementCounter(); @@ -76,13 +86,14 @@ public UGICacheEntry getTimedProxyUGI(SessionId session) private void cleanup(Integer segmentId) { UGICacheEntry ugi = null; - while ((ugi = delayQueues[segmentId].poll()) != null) { + DelayQueue delayQueue = getDelayQueue(segmentId); + while ((ugi = delayQueue.poll()) != null) { // Place it back in the queue if still in use and was not closed if (!closeUGI(ugi)) { - delayQueues[segmentId].offer(ugi); + delayQueue.offer(ugi); } LOG.debug("Delay Queue Size for segment " + - segmentId + " = " + delayQueues[segmentId].size()); + segmentId + " = " + delayQueue.size()); } } @@ -93,7 +104,7 @@ public void release(UGICacheEntry timedProxyUGI, boolean forceClean) { timedProxyUGI.resetTime(); timedProxyUGI.decrementCounter(); if (forceClean) { - synchronized (delayQueues[segmentId]) { + synchronized (getDelayQueue(segmentId)) { timedProxyUGI.setExpired(); closeUGI(timedProxyUGI); } diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index c0e3881325..e81888a94d 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -85,6 +85,13 @@ public void getUGIWhenRequestedUserDoesNotExist() throws Exception { // how about getLoginUser()? } + @Test + public void anySegmentIdIsValid() throws Exception { + session = new SessionId(65, "txn-id", "the-user"); + UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + assertNotNull(proxyUGI1.getUGI()); + } + @Test public void releaseWithoutForceClean() throws Exception { UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); From b9cf8b05df05659445d9818ca592fa0b5eea676c Mon Sep 17 00:00:00 2001 From: Lav Jain Date: Tue, 10 Jul 2018 16:21:11 -0700 Subject: [PATCH 10/28] Change lambda to anonymous class for Java 7 --- .../java/org/apache/hawq/pxf/service/UGICacheTest.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index e81888a94d..e1c33838fa 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -3,6 +3,7 @@ import org.apache.hadoop.security.UserGroupInformation; import org.junit.Before; import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import static org.junit.Assert.assertEquals; @@ -20,11 +21,14 @@ public class UGICacheTest { @Before public void setUp() throws Exception { provider = mock(UGIProvider.class); - - when(provider.createProxyUGI(any(String.class))).thenAnswer((Answer) invocation -> mock(UserGroupInformation.class)); + when(provider.createProxyUGI(any(String.class))).thenAnswer(new Answer() { + @Override + public UserGroupInformation answer(InvocationOnMock invocation) throws Throwable { + return mock(UserGroupInformation.class); + } + }); session = new SessionId(0, "txn-id", "the-user"); - cache = new UGICache(provider); } From daf1678529675f3d6a91c866519156ba923b9606 Mon Sep 17 00:00:00 2001 From: Alex Denissov Date: Tue, 10 Jul 2018 16:55:00 -0700 Subject: [PATCH 11/28] Fix warnings - Made public methods private or package private where possible Co-authored-by: Lav Jain Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 4 ++-- .../hawq/pxf/service/UGICacheEntry.java | 20 +++++++++---------- .../apache/hawq/pxf/service/UGIProvider.java | 10 +++++----- .../servlet/SecurityServletFilter.java | 6 ++---- .../apache/hawq/pxf/service/UGICacheTest.java | 7 +++++-- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index a61bd99247..3634a3f155 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -37,7 +37,7 @@ public class UGICache { private final Map> queueMap = new HashMap<>(); private final UGIProvider ugiProvider; - public UGICache(UGIProvider provider) { + UGICache(UGIProvider provider) { this.ugiProvider = provider; } @@ -85,7 +85,7 @@ public UGICacheEntry getTimedProxyUGI(SessionId session) // and clean them if possible private void cleanup(Integer segmentId) { - UGICacheEntry ugi = null; + UGICacheEntry ugi; DelayQueue delayQueue = getDelayQueue(segmentId); while ((ugi = delayQueue.poll()) != null) { // Place it back in the queue if still in use and was not closed diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java index 7cab5ff57f..f0cf8efb3b 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java @@ -34,7 +34,7 @@ public class UGICacheEntry implements Delayed { private AtomicInteger inProgress = new AtomicInteger(); private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes - public UGICacheEntry(UserGroupInformation proxyUGI, SessionId session) { + UGICacheEntry(UserGroupInformation proxyUGI, SessionId session) { this.startTime = System.currentTimeMillis(); this.proxyUGI = proxyUGI; this.session = session; @@ -44,35 +44,35 @@ public UserGroupInformation getUGI() { return proxyUGI; } - public SessionId getSession() { + SessionId getSession() { return session; } - public boolean isCleaned() { + boolean isCleaned() { return cleaned; } - public void setCleaned() { + void setCleaned() { cleaned = true; } - public int getCounter() { + int getCounter() { return inProgress.get(); } - public void incrementCounter() { + void incrementCounter() { inProgress.incrementAndGet(); } - public void decrementCounter() { + void decrementCounter() { inProgress.decrementAndGet(); } - public void resetTime() { + void resetTime() { startTime = System.currentTimeMillis(); } - public void setExpired() { + void setExpired() { startTime = -1L; } @@ -87,7 +87,7 @@ public int compareTo(Delayed other) { return Long.compare(this.getDelayMillis(), that.getDelayMillis()); } - public long getDelayMillis() { + private long getDelayMillis() { return (startTime + UGI_CACHE_EXPIRY) - System.currentTimeMillis(); } } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java index 69a007c626..1dae2a1341 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java @@ -24,14 +24,14 @@ import java.io.IOException; -public class UGIProvider { - - public UserGroupInformation createProxyUGI(String effectiveUser) throws IOException { +class UGIProvider { + + UserGroupInformation createProxyUGI(String effectiveUser) throws IOException { return UserGroupInformation.createProxyUser( effectiveUser, UserGroupInformation.getLoginUser()); } - - public void destroy(UserGroupInformation ugi) throws IOException { + + void destroy(UserGroupInformation ugi) throws IOException { FileSystem.closeAllForUGI(ugi); } } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 1426668c8d..b11b453516 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -88,9 +88,7 @@ public void doFilter(final ServletRequest request, final ServletResponse respons SessionId session = new SessionId(segmentId, transactionId, user); if (LOG.isDebugEnabled() && fragmentCount != null) { - StringBuilder sb = new StringBuilder(session.toString()); - sb.append(" Fragment = ").append(fragmentIndex).append(" of ").append(fragmentCount); - LOG.debug(sb.toString()); + LOG.debug(session.toString() + " Fragment = " + fragmentIndex + " of " + fragmentCount); } // TODO refresh Kerberos token when security is enabled @@ -117,7 +115,7 @@ public Boolean run() throws IOException, ServletException { } finally { // Optimization to cleanup the cache if it is the last fragment - boolean forceClean = (fragmentIndex != null && fragmentCount.equals(fragmentIndex)); + boolean forceClean = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); cache.release(timedProxyUGI, forceClean); } } else { diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index e1c33838fa..958ca92106 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -10,7 +10,10 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.mockito.Matchers.any; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.powermock.api.mockito.PowerMockito.when; public class UGICacheTest { @@ -23,7 +26,7 @@ public void setUp() throws Exception { provider = mock(UGIProvider.class); when(provider.createProxyUGI(any(String.class))).thenAnswer(new Answer() { @Override - public UserGroupInformation answer(InvocationOnMock invocation) throws Throwable { + public UserGroupInformation answer(InvocationOnMock invocation) { return mock(UserGroupInformation.class); } }); From db46b74cc4bbce3aaffc8c6301a35bf255273132 Mon Sep 17 00:00:00 2001 From: Lav Jain Date: Tue, 10 Jul 2018 16:55:58 -0700 Subject: [PATCH 12/28] Make UGI cache field of SecurityServletFilter final - This may fix a NPE we were seeing during the smoketest Co-authored-by: Lav Jain Co-authored-by: Ben Christel --- .../apache/hawq/pxf/service/servlet/SecurityServletFilter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index b11b453516..ef4b2ec937 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -53,7 +53,7 @@ public class SecurityServletFilter implements Filter { private static final String FRAGMENT_COUNT_HEADER = "X-GP-FRAGMENT-COUNT"; private static final String MISSING_HEADER_ERROR = "Header %s is missing in the request"; private static final String EMPTY_HEADER_ERROR = "Header %s is empty in the request"; - private static UGICache cache = new UGICache(); + private static final UGICache cache = new UGICache(); /** * Initializes the filter. From 08dabd2e2e02996b693ecb6496a27cdd34d192f2 Mon Sep 17 00:00:00 2001 From: Alex Denissov Date: Tue, 10 Jul 2018 17:15:37 -0700 Subject: [PATCH 13/28] Add Apache Licence to UGICacheTest file Co-authored-by: Lav Jain Co-authored-by: Ben Christel --- .../apache/hawq/pxf/service/UGICacheTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index 958ca92106..69d7da3269 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -1,5 +1,24 @@ package org.apache.hawq.pxf.service; +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + import org.apache.hadoop.security.UserGroupInformation; import org.junit.Before; import org.junit.Test; From d9552f2c2089dc05bd96164019092144e1c00bc9 Mon Sep 17 00:00:00 2001 From: Lav Jain Date: Wed, 11 Jul 2018 12:13:14 -0700 Subject: [PATCH 14/28] Add JavaDoc comments for UGICache and related classes - Make UGICacheEntry an inner class of UGICache and rename to Entry Co-authored-by: Ben Christel Co-authored-by: Francisco Guerrero --- .../apache/hawq/pxf/service/SessionId.java | 30 +++ .../org/apache/hawq/pxf/service/UGICache.java | 250 ++++++++++++++---- .../hawq/pxf/service/UGICacheEntry.java | 94 ------- .../apache/hawq/pxf/service/UGIProvider.java | 17 ++ .../servlet/SecurityServletFilter.java | 14 +- .../apache/hawq/pxf/service/UGICacheTest.java | 76 +++--- 6 files changed, 301 insertions(+), 180 deletions(-) delete mode 100644 pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java index f827043e96..0b2dbf70ad 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java @@ -19,27 +19,51 @@ * under the License. */ +/** + * For the purposes of pxf-server, a session is the set of requests processed on a specific segment + * on behalf of a particular user and transaction. Grouping requests together into a session allows + * us to re-use the UserGroupInformation object (which is expensive to destroy) for each session. + * + * SessionId is used as the cache key to look up the UserGroupInformation for a request. See {@link + * UGICache}. + */ public class SessionId { private final String user; private Integer segmentId; private String sessionId; + /** + * Create a sessionId + * + * @param segmentId + * @param transactionId + * @param gpdbUser + */ public SessionId(Integer segmentId, String transactionId, String gpdbUser) { this.segmentId = segmentId; this.user = gpdbUser; this.sessionId = segmentId + ":" + transactionId + ":" + gpdbUser; } + /** + * @return the segment id + */ public Integer getSegmentId() { return segmentId; } + /** + * {@inheritDoc} + */ @Override public int hashCode() { return sessionId.hashCode(); } + /** + * {@inheritDoc} + */ @Override public boolean equals(Object other) { if (!(other instanceof SessionId)) { @@ -49,11 +73,17 @@ public boolean equals(Object other) { return this.sessionId.equals(that.sessionId); } + /** + * {@inheritDoc} + */ @Override public String toString() { return "Session = " + sessionId; } + /** + * @return the gpdb user name + */ public String getUser() { return user; } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 3634a3f155..d73f28acb0 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -24,72 +24,125 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.DelayQueue; +import java.util.concurrent.Delayed; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.security.UserGroupInformation; +/** + * Stores UserGroupInformation instances for each active session. The UGIs are cleaned up if they + * have not been accessed for 15 minutes. + * + * The motivation for caching is that destroying UGIs is slow. The alternative, creating and + * destroying a UGI per-request, is wasteful. + */ public class UGICache { private static final Log LOG = LogFactory.getLog(UGICache.class); - private Map cache = new ConcurrentHashMap<>(); + private Map cache = new ConcurrentHashMap<>(); @SuppressWarnings("unchecked") // There is a separate DelayQueue for each segment (also being used for locking) - private final Map> queueMap = new HashMap<>(); + private final Map> queueMap = new HashMap<>(); private final UGIProvider ugiProvider; + /** + * Create a UGICache with the given {@link UGIProvider}. Intended for use by tests which need + * to substitute a mock UGIProvider. + */ UGICache(UGIProvider provider) { this.ugiProvider = provider; } + /** + * Create a UGICache. Automatically creates a {@link UGIProvider} that this cache will use to + * create and destroy UserGroupInformation instances. + */ public UGICache() { this(new UGIProvider()); } - private DelayQueue getDelayQueue(Integer segmentId) { - DelayQueue queue = queueMap.get(segmentId); - if (queue == null) { - synchronized (queueMap) { - queue = queueMap.get(segmentId); - if (queue == null) { - queue = new DelayQueue<>(); - queueMap.put(segmentId, queue); - } - } - } - return queue; - } - - // Create new proxy UGI if not found in cache and increment reference count - public UGICacheEntry getTimedProxyUGI(SessionId session) + /** + * Create new proxy UGI if not found in cache and increment reference count + */ + public Entry getTimedProxyUGI(SessionId session) throws IOException { Integer segmentId = session.getSegmentId(); String user = session.getUser(); - DelayQueue delayQueue = getDelayQueue(segmentId); + DelayQueue delayQueue = getExpirationQueue(segmentId); synchronized (delayQueue) { // Use the opportunity to cleanup any expired entries cleanup(segmentId); - UGICacheEntry timedProxyUGI = cache.get(session); - if (timedProxyUGI == null) { + Entry entry = cache.get(session); + if (entry == null) { LOG.info(session.toString() + " Creating proxy user = " + user); - timedProxyUGI = new UGICacheEntry(ugiProvider.createProxyUGI(user), session); - delayQueue.offer(timedProxyUGI); - cache.put(session, timedProxyUGI); + entry = new Entry(ugiProvider.createProxyUGI(user), session); + delayQueue.offer(entry); + cache.put(session, entry); } - timedProxyUGI.incrementCounter(); - return timedProxyUGI; + entry.acquireReference(); + return entry; } } - // Poll segment expiration queue for all expired entries - // and clean them if possible + /** + * Decrement reference count for the given Entry. + * + * @param timedProxyUGI the cache entry to release + * @param forceClean if true, destroys the UGI for the given Entry (only if it is now unreferenced). + */ + public void release(Entry timedProxyUGI, boolean forceClean) { + + Integer segmentId = timedProxyUGI.getSession().getSegmentId(); + timedProxyUGI.resetTime(); + timedProxyUGI.releaseReference(); + if (forceClean) { + synchronized (getExpirationQueue(segmentId)) { + timedProxyUGI.setExpired(); + closeUGI(timedProxyUGI); + } + } + } + + /** + * Get the queue of cache entries associated with a segment, creating it if it doesn't + * yet exist. This lets us lazily populate the queueMap. + * + * @param segmentId + * @return the {@link DelayQueue} associated to the segment. + */ + private DelayQueue getExpirationQueue(Integer segmentId) { + DelayQueue queue = queueMap.get(segmentId); + if (queue == null) { + synchronized (queueMap) { + queue = queueMap.get(segmentId); + if (queue == null) { + queue = new DelayQueue<>(); + queueMap.put(segmentId, queue); + } + } + } + return queue; + } + + /** + * Iterate through all the entries in the queue for the given segment + * and close expired {@link UserGroupInformation}, otherwise it resets + * the timer for every non-expired entry. + * + * @param segmentId + */ private void cleanup(Integer segmentId) { - UGICacheEntry ugi; - DelayQueue delayQueue = getDelayQueue(segmentId); + Entry ugi; + DelayQueue delayQueue = getExpirationQueue(segmentId); while ((ugi = delayQueue.poll()) != null) { // Place it back in the queue if still in use and was not closed if (!closeUGI(ugi)) { + ugi.resetTime(); delayQueue.offer(ugi); } LOG.debug("Delay Queue Size for segment " + @@ -97,39 +150,25 @@ private void cleanup(Integer segmentId) { } } - // Decrement reference count and optional cleanup - public void release(UGICacheEntry timedProxyUGI, boolean forceClean) { - - Integer segmentId = timedProxyUGI.getSession().getSegmentId(); - timedProxyUGI.resetTime(); - timedProxyUGI.decrementCounter(); - if (forceClean) { - synchronized (getDelayQueue(segmentId)) { - timedProxyUGI.setExpired(); - closeUGI(timedProxyUGI); - } - } - } - // There is no need to synchronize this method because it should be called // from within a synchronized block - private boolean closeUGI(UGICacheEntry expiredProxyUGI) { + private boolean closeUGI(Entry expiredProxyUGI) { SessionId session = expiredProxyUGI.getSession(); String fsMsg = "FileSystem for proxy user = " + expiredProxyUGI.getUGI().getUserName(); try { // The UGI object is still being used by another thread - if (expiredProxyUGI.getCounter() != 0) { + if (expiredProxyUGI.countReferences() != 0) { LOG.info(session.toString() + " Skipping close of " + fsMsg); // Reset time so that it doesn't expire until release // updates the time again - expiredProxyUGI.resetTime(); +// expiredProxyUGI.resetTime(); return false; } // Expired UGI object can be cleaned since it is not used // Determine if it can be removed from cache also - UGICacheEntry cachedUGI = cache.get(session); + Entry cachedUGI = cache.get(session); if (expiredProxyUGI == cachedUGI) { // Remove it from cache, as cache now has an // expired entry which is not in progress @@ -151,4 +190,119 @@ private boolean closeUGI(UGICacheEntry expiredProxyUGI) { return true; } + /** + * Stores a {@link UserGroupInformation} associated with a {@link SessionId}, and determines + * when to expire the UGI. + */ + public static class Entry implements Delayed { + + private volatile long startTime; + private UserGroupInformation proxyUGI; + private SessionId session; + private boolean cleaned = false; + private AtomicInteger referenceCount = new AtomicInteger(); + private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes + + /** + * Creates a new UGICache Entry. + * + * @param proxyUGI + * @param session + */ + Entry(UserGroupInformation proxyUGI, SessionId session) { + this.startTime = System.currentTimeMillis(); + this.proxyUGI = proxyUGI; + this.session = session; + } + + /** + * @return the Cached {@link UserGroupInformation}. + */ + public UserGroupInformation getUGI() { + return proxyUGI; + } + + /** + * @return the {@link SessionId}. + */ + SessionId getSession() { + return session; + } + + /** + * @return true if the setCleaned has been invoked, false otherwise. + */ + boolean isCleaned() { + return cleaned; + } + + /** + * mark that the {@link UserGroupInformation} in this Entry has been destroyed. + */ + void setCleaned() { + cleaned = true; + } + + /** + * @return the number of active requests using the {@link UserGroupInformation}. + */ + int countReferences() { + return referenceCount.get(); + } + + /** + * Increments the number of references accessing the {@link UserGroupInformation}. + */ + void acquireReference() { + referenceCount.incrementAndGet(); + } + + /** + * Decrements the number of references accessing the {@link UserGroupInformation}. + */ + void releaseReference() { + referenceCount.decrementAndGet(); + } + + /** + * Resets the timer for removing this Entry from the cache. + */ + void resetTime() { + startTime = System.currentTimeMillis(); + } + + /** + * Expire the UGICache Entry + */ + void setExpired() { + startTime = -1L; + } + + /** + * {@inheritDoc} + */ + @Override + public long getDelay(TimeUnit unit) { + return unit.convert(getDelayMillis(), TimeUnit.MILLISECONDS); + } + + /** + * Compare the expiry time of this cache entry to another cache entry's expiry time. + * + * @param other a UGICache.Entry (passing any other kind of Delayed produces an error) + * @see java.lang.Comparable<>#compareTo(java.lang.Comparable<>) + */ + @Override + public int compareTo(Delayed other) { + Entry that = (Entry) other; + return Long.compare(this.getDelayMillis(), that.getDelayMillis()); + } + + /** + * @return the number of milliseconds remaining before this cache entry expires. + */ + private long getDelayMillis() { + return (startTime + UGI_CACHE_EXPIRY) - System.currentTimeMillis(); + } + } } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java deleted file mode 100644 index f0cf8efb3b..0000000000 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICacheEntry.java +++ /dev/null @@ -1,94 +0,0 @@ -package org.apache.hawq.pxf.service; - -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.Delayed; -import java.util.concurrent.TimeUnit; - -import org.apache.hadoop.security.UserGroupInformation; - -public class UGICacheEntry implements Delayed { - - private volatile long startTime; - private UserGroupInformation proxyUGI; - private SessionId session; - private boolean cleaned = false; - private AtomicInteger inProgress = new AtomicInteger(); - private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes - - UGICacheEntry(UserGroupInformation proxyUGI, SessionId session) { - this.startTime = System.currentTimeMillis(); - this.proxyUGI = proxyUGI; - this.session = session; - } - - public UserGroupInformation getUGI() { - return proxyUGI; - } - - SessionId getSession() { - return session; - } - - boolean isCleaned() { - return cleaned; - } - - void setCleaned() { - cleaned = true; - } - - int getCounter() { - return inProgress.get(); - } - - void incrementCounter() { - inProgress.incrementAndGet(); - } - - void decrementCounter() { - inProgress.decrementAndGet(); - } - - void resetTime() { - startTime = System.currentTimeMillis(); - } - - void setExpired() { - startTime = -1L; - } - - @Override - public long getDelay(TimeUnit unit) { - return unit.convert(getDelayMillis(), TimeUnit.MILLISECONDS); - } - - @Override - public int compareTo(Delayed other) { - UGICacheEntry that = (UGICacheEntry) other; - return Long.compare(this.getDelayMillis(), that.getDelayMillis()); - } - - private long getDelayMillis() { - return (startTime + UGI_CACHE_EXPIRY) - System.currentTimeMillis(); - } -} - diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java index 1dae2a1341..38c60a0403 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGIProvider.java @@ -24,13 +24,30 @@ import java.io.IOException; +/** + * Thin wrapper around {@link UserGroupInformation} create and destroy methods. We mock this class + * in tests to be able to detect when a UGI is created/destroyed, and to isolate our tests from + * creating/destroying real UGI instances. + */ class UGIProvider { + /** + * Wrapper for {@link UserGroupInformation} creation + * + * @param effectiveUser the name of the user that we want to impersonate + * @return a {@link UserGroupInformation} for impersonation. + * @throws IOException + */ UserGroupInformation createProxyUGI(String effectiveUser) throws IOException { return UserGroupInformation.createProxyUser( effectiveUser, UserGroupInformation.getLoginUser()); } + /** + * Wrapper for {@link FileSystem}.closeAllForUGI method. + * @param ugi the {@link UserGroupInformation} whose filesystem resources we want to free. + * @throws IOException + */ void destroy(UserGroupInformation ugi) throws IOException { FileSystem.closeAllForUGI(ugi); } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index ef4b2ec937..d4265ca7c4 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -34,8 +34,8 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.security.UserGroupInformation; import org.apache.hawq.pxf.service.SessionId; -import org.apache.hawq.pxf.service.UGICacheEntry; import org.apache.hawq.pxf.service.UGICache; import org.apache.hawq.pxf.service.utilities.SecureLogin; @@ -62,6 +62,7 @@ public class SecurityServletFilter implements Filter { */ @Override public void init(FilterConfig filterConfig) throws ServletException { + //TODO: initialize cache here } /** @@ -80,13 +81,13 @@ public void doFilter(final ServletRequest request, final ServletResponse respons if (SecureLogin.isUserImpersonationEnabled()) { // retrieve user header and make sure header is present and is not empty - final String user = getHeaderValue(request, USER_HEADER); + final String gpdbUser = getHeaderValue(request, USER_HEADER); String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER); Integer segmentId = getHeaderValueInt(request, SEGMENT_ID_HEADER, true); Integer fragmentCount = getHeaderValueInt(request, FRAGMENT_COUNT_HEADER, false); Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); - SessionId session = new SessionId(segmentId, transactionId, user); + SessionId session = new SessionId(segmentId, transactionId, gpdbUser); if (LOG.isDebugEnabled() && fragmentCount != null) { LOG.debug(session.toString() + " Fragment = " + fragmentIndex + " of " + fragmentCount); } @@ -97,14 +98,14 @@ public void doFilter(final ServletRequest request, final ServletResponse respons PrivilegedExceptionAction action = new PrivilegedExceptionAction() { @Override public Boolean run() throws IOException, ServletException { - LOG.debug("Performing request chain call for proxy user = " + user); + LOG.debug("Performing request chain call for proxy user = " + gpdbUser); chain.doFilter(request, response); return true; } }; // create proxy user UGI from the UGI of the logged in user and execute the servlet chain as that user - UGICacheEntry timedProxyUGI = cache.getTimedProxyUGI(session); + UGICache.Entry timedProxyUGI = cache.getTimedProxyUGI(session); try { timedProxyUGI.getUGI().doAs(action); } catch (UndeclaredThrowableException ute) { @@ -112,8 +113,7 @@ public Boolean run() throws IOException, ServletException { throw new ServletException(ute.getCause()); } catch (InterruptedException ie) { throw new ServletException(ie); - } - finally { + } finally { // Optimization to cleanup the cache if it is the last fragment boolean forceClean = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); cache.release(timedProxyUGI, forceClean); diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index 69d7da3269..ca3dff250b 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -28,6 +28,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -56,15 +57,15 @@ public UserGroupInformation answer(InvocationOnMock invocation) { @Test public void getUGIFromEmptyCache() throws Exception { - UGICacheEntry entry = cache.getTimedProxyUGI(session); + UGICache.Entry entry = cache.getTimedProxyUGI(session); assertNotNull(entry.getUGI()); verify(provider).createProxyUGI("the-user"); } @Test public void getSameUGITwiceUsesCache() throws Exception { - UGICacheEntry entry1 = cache.getTimedProxyUGI(session); - UGICacheEntry entry2 = cache.getTimedProxyUGI(session); + UGICache.Entry entry1 = cache.getTimedProxyUGI(session); + UGICache.Entry entry2 = cache.getTimedProxyUGI(session); assertEquals(entry1, entry2); assertNotNull(entry1.getUGI()); verify(provider, times(1)).createProxyUGI("the-user"); @@ -73,8 +74,8 @@ public void getSameUGITwiceUsesCache() throws Exception { @Test public void getTwoUGIsWithDifferentSessionsForSameUser() throws Exception { SessionId otherSession = new SessionId(0, "txn-id-2", "the-user"); - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(otherSession); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(otherSession); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(2)).createProxyUGI("the-user"); // TODO: this seems weird. We're creating two UGIs with the same params, @@ -84,8 +85,8 @@ public void getTwoUGIsWithDifferentSessionsForSameUser() throws Exception { @Test public void getTwoUGIsWithDifferentUsers() throws Exception { SessionId otherSession = new SessionId(0, "txn-id", "different-user"); - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(otherSession); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(otherSession); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(1)).createProxyUGI("the-user"); verify(provider, times(1)).createProxyUGI("different-user"); @@ -94,10 +95,10 @@ public void getTwoUGIsWithDifferentUsers() throws Exception { @Test public void getTwoUGIsWithDifferentUsersCachesBoth() throws Exception { SessionId otherSession = new SessionId(0, "txn-id", "different-user"); - UGICacheEntry proxyUGI1a = cache.getTimedProxyUGI(session); - UGICacheEntry proxyUGI1b = cache.getTimedProxyUGI(session); - UGICacheEntry proxyUGI2a = cache.getTimedProxyUGI(otherSession); - UGICacheEntry proxyUGI2b = cache.getTimedProxyUGI(otherSession); + UGICache.Entry proxyUGI1a = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1b = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2a = cache.getTimedProxyUGI(otherSession); + UGICache.Entry proxyUGI2b = cache.getTimedProxyUGI(otherSession); assertEquals(proxyUGI1a, proxyUGI1b); assertEquals(proxyUGI2a, proxyUGI2b); assertNotEquals(proxyUGI1a, proxyUGI2a); @@ -114,39 +115,52 @@ public void getUGIWhenRequestedUserDoesNotExist() throws Exception { @Test public void anySegmentIdIsValid() throws Exception { session = new SessionId(65, "txn-id", "the-user"); - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); assertNotNull(proxyUGI1.getUGI()); } @Test public void releaseWithoutForceClean() throws Exception { - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); cache.release(proxyUGI1, false); // UGI wasn't cleaned up, so we can still get it - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); assertEquals(proxyUGI1, proxyUGI2); verify(provider, times(1)).createProxyUGI("the-user"); } @Test public void releaseWithForceClean() throws Exception { - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); cache.release(proxyUGI1, true); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(2)).createProxyUGI("the-user"); } + @Test + public void releaseWithForceCleanResetsTheExpirationTimeIfUGIIsReferenced() throws Exception { + UGICache.Entry reference1 = cache.getTimedProxyUGI(session); + UGICache.Entry reference2 = cache.getTimedProxyUGI(session); + + cache.release(reference1, true); + cache.release(reference2, false); + + UGICache.Entry reference3 = cache.getTimedProxyUGI(session); + + assertEquals(reference1, reference3); + } + @Test public void releaseAndReacquireDoesNotFreeResources() throws Exception { - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); cache.release(proxyUGI1, false); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); proxyUGI2.setExpired(); - UGICacheEntry proxyUGI3 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI3 = cache.getTimedProxyUGI(session); // this does not clean up any UGIs because our proxyUGI is still in use. assertEquals(proxyUGI3, proxyUGI2); verify(provider, times(1)).createProxyUGI("the-user"); @@ -155,12 +169,12 @@ public void releaseAndReacquireDoesNotFreeResources() throws Exception { @Test public void releaseAndAcquireAfterTimeoutFreesResources() throws Exception { - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); cache.release(proxyUGI1, false); proxyUGI1.setExpired(); verify(provider, never()).destroy(any(UserGroupInformation.class)); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); verify(provider).destroy(proxyUGI1.getUGI()); assertNotEquals(proxyUGI2, proxyUGI1); assertNotEquals(proxyUGI2.getUGI(), proxyUGI1.getUGI()); @@ -172,22 +186,22 @@ public void releaseAndAcquireAfterTimeoutFreesResources() throws Exception { @Test public void releaseAnExpiredUGIResetsTheTimer() throws Exception { - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); proxyUGI1.setExpired(); cache.release(proxyUGI1, false); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); assertEquals(proxyUGI2, proxyUGI1); verify(provider, never()).destroy(any(UserGroupInformation.class)); } @Test public void releaseDoesNotFreeResourcesIfUGIIsUsedElsewhere() throws Exception { - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); cache.release(proxyUGI1, true); // UGI was not cleaned up because proxyUGI2 is referencing it - UGICacheEntry proxyUGI3 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI3 = cache.getTimedProxyUGI(session); assertEquals(proxyUGI1, proxyUGI2); assertEquals(proxyUGI1, proxyUGI3); verify(provider, times(1)).createProxyUGI("the-user"); @@ -195,14 +209,14 @@ public void releaseDoesNotFreeResourcesIfUGIIsUsedElsewhere() throws Exception { @Test public void releasingAllReferencesFreesResources() throws Exception { - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); cache.release(proxyUGI1, true); cache.release(proxyUGI2, true); // at this point, the initial UGI has been freed. Calling // getTimedProxyUGI again creates a new UGI. - UGICacheEntry proxyUGI3 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI3 = cache.getTimedProxyUGI(session); assertEquals(proxyUGI1, proxyUGI2); assertNotEquals(proxyUGI1, proxyUGI3); verify(provider, times(2)).createProxyUGI("the-user"); @@ -212,11 +226,11 @@ public void releasingAllReferencesFreesResources() throws Exception { public void releaseAnEntryNotInTheCache() throws Exception { // this could happen if some caller of the cache // retains a reference to a cache entry after releasing it. - UGICacheEntry proxyUGI1 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); cache.release(proxyUGI1, true); cache.release(proxyUGI1, true); - UGICacheEntry proxyUGI2 = cache.getTimedProxyUGI(session); + UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(2)).createProxyUGI("the-user"); } From a7460c3da01a415ab2835c6448ec438121d083fd Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Wed, 11 Jul 2018 16:55:42 -0700 Subject: [PATCH 15/28] Refactored UGICacheEntry to live inside UGICache and be only accessible by inside UGICache. - Mock guava's Ticker and use it in UGICache. - Returning an UGI from the UGICache instead of the UGICache.Entry Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../apache/hawq/pxf/service/SessionId.java | 39 ++-- .../org/apache/hawq/pxf/service/UGICache.java | 128 +++++++------ .../servlet/SecurityServletFilter.java | 11 +- .../apache/hawq/pxf/service/UGICacheTest.java | 176 ++++++++++-------- 4 files changed, 193 insertions(+), 161 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java index 0b2dbf70ad..f0f3e9b232 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java @@ -19,19 +19,21 @@ * under the License. */ +import org.apache.commons.lang3.builder.EqualsBuilder; + /** * For the purposes of pxf-server, a session is the set of requests processed on a specific segment * on behalf of a particular user and transaction. Grouping requests together into a session allows * us to re-use the UserGroupInformation object (which is expensive to destroy) for each session. - * + *

* SessionId is used as the cache key to look up the UserGroupInformation for a request. See {@link * UGICache}. */ public class SessionId { private final String user; - private Integer segmentId; - private String sessionId; + private final Integer segmentId; + private final String sessionId; /** * Create a sessionId @@ -43,7 +45,7 @@ public class SessionId { public SessionId(Integer segmentId, String transactionId, String gpdbUser) { this.segmentId = segmentId; this.user = gpdbUser; - this.sessionId = segmentId + ":" + transactionId + ":" + gpdbUser; + this.sessionId = gpdbUser + ":" + transactionId + ":" + segmentId; } /** @@ -53,6 +55,13 @@ public Integer getSegmentId() { return segmentId; } + /** + * @return the gpdb user name + */ + public String getUser() { + return user; + } + /** * {@inheritDoc} */ @@ -65,12 +74,15 @@ public int hashCode() { * {@inheritDoc} */ @Override - public boolean equals(Object other) { - if (!(other instanceof SessionId)) { - return false; - } - SessionId that = (SessionId) other; - return this.sessionId.equals(that.sessionId); + public boolean equals(Object obj) { + if (obj == null) return false; + if (obj == this) return true; + if (obj.getClass() != getClass()) return false; + + SessionId that = (SessionId) obj; + return new EqualsBuilder() + .append(sessionId, that.sessionId) + .isEquals(); } /** @@ -80,11 +92,4 @@ public boolean equals(Object other) { public String toString() { return "Session = " + sessionId; } - - /** - * @return the gpdb user name - */ - public String getUser() { - return user; - } } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index d73f28acb0..5d523cdbe1 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -28,6 +28,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import com.google.common.base.Ticker; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.security.UserGroupInformation; @@ -35,7 +36,7 @@ /** * Stores UserGroupInformation instances for each active session. The UGIs are cleaned up if they * have not been accessed for 15 minutes. - * + *

* The motivation for caching is that destroying UGIs is slow. The alternative, creating and * destroying a UGI per-request, is wasteful. */ @@ -47,12 +48,14 @@ public class UGICache { // There is a separate DelayQueue for each segment (also being used for locking) private final Map> queueMap = new HashMap<>(); private final UGIProvider ugiProvider; + private Ticker ticker; /** * Create a UGICache with the given {@link UGIProvider}. Intended for use by tests which need * to substitute a mock UGIProvider. */ - UGICache(UGIProvider provider) { + UGICache(UGIProvider provider, Ticker ticker) { + this.ticker = ticker; this.ugiProvider = provider; } @@ -61,48 +64,51 @@ public class UGICache { * create and destroy UserGroupInformation instances. */ public UGICache() { - this(new UGIProvider()); + this(new UGIProvider(), Ticker.systemTicker()); } /** * Create new proxy UGI if not found in cache and increment reference count */ - public Entry getTimedProxyUGI(SessionId session) - throws IOException { - + public UserGroupInformation getUserGroupInformation(SessionId session) throws IOException { Integer segmentId = session.getSegmentId(); String user = session.getUser(); DelayQueue delayQueue = getExpirationQueue(segmentId); synchronized (delayQueue) { // Use the opportunity to cleanup any expired entries - cleanup(segmentId); + cleanup(session); Entry entry = cache.get(session); if (entry == null) { LOG.info(session.toString() + " Creating proxy user = " + user); - entry = new Entry(ugiProvider.createProxyUGI(user), session); + entry = new Entry(ticker, ugiProvider.createProxyUGI(user)); delayQueue.offer(entry); cache.put(session, entry); } entry.acquireReference(); - return entry; + return entry.getUGI(); } } /** - * Decrement reference count for the given Entry. + * Decrement reference count for the given session's UGI. Resets the time at which the UGI will + * expire to 15 minutes in the future. * - * @param timedProxyUGI the cache entry to release - * @param forceClean if true, destroys the UGI for the given Entry (only if it is now unreferenced). + * @param session the session for which we want to release the UGI. + * @param cleanImmediatelyIfNoRefs if true, destroys the UGI for the given session (only if it is + * now unreferenced). */ - public void release(Entry timedProxyUGI, boolean forceClean) { + public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { + + Entry timedProxyUGI = cache.get(session); + + if (timedProxyUGI == null) return; - Integer segmentId = timedProxyUGI.getSession().getSegmentId(); timedProxyUGI.resetTime(); timedProxyUGI.releaseReference(); - if (forceClean) { - synchronized (getExpirationQueue(segmentId)) { + if (cleanImmediatelyIfNoRefs) { + synchronized (getExpirationQueue(session.getSegmentId())) { timedProxyUGI.setExpired(); - closeUGI(timedProxyUGI); + closeUGI(session, timedProxyUGI); } } } @@ -129,47 +135,52 @@ private DelayQueue getExpirationQueue(Integer segmentId) { } /** - * Iterate through all the entries in the queue for the given segment + * Iterate through all the entries in the queue for the given session's segment * and close expired {@link UserGroupInformation}, otherwise it resets * the timer for every non-expired entry. * - * @param segmentId + * @param session */ - private void cleanup(Integer segmentId) { + private void cleanup(SessionId session) { Entry ugi; - DelayQueue delayQueue = getExpirationQueue(segmentId); + DelayQueue delayQueue = getExpirationQueue(session.getSegmentId()); while ((ugi = delayQueue.poll()) != null) { // Place it back in the queue if still in use and was not closed - if (!closeUGI(ugi)) { + if (!closeUGI(session, ugi)) { ugi.resetTime(); delayQueue.offer(ugi); } LOG.debug("Delay Queue Size for segment " + - segmentId + " = " + delayQueue.size()); + session.getSegmentId() + " = " + delayQueue.size()); } } - // There is no need to synchronize this method because it should be called - // from within a synchronized block - private boolean closeUGI(Entry expiredProxyUGI) { - - SessionId session = expiredProxyUGI.getSession(); - String fsMsg = "FileSystem for proxy user = " + expiredProxyUGI.getUGI().getUserName(); + /** + * This method must be called from a synchronized block for the delayQueue for the given + * session.getSegmentId(). When the reference count is 0, the Entry is removed from the + * cache where it will then be processed to be destroyed by the UGIProvider. + * + * @param session + * @param toDelete + * @return true if the UGI entry was cleaned, false when the UGI entry was still in use + * and cleaning up was skipped + */ + private boolean closeUGI(SessionId session, Entry toDelete) { + // There is no need to synchronize this method because it should be called + // from within a synchronized block + String fsMsg = "FileSystem for proxy user = " + toDelete.getUGI().getUserName(); try { // The UGI object is still being used by another thread - if (expiredProxyUGI.countReferences() != 0) { + if (toDelete.isInUse()) { LOG.info(session.toString() + " Skipping close of " + fsMsg); - // Reset time so that it doesn't expire until release - // updates the time again -// expiredProxyUGI.resetTime(); return false; } // Expired UGI object can be cleaned since it is not used // Determine if it can be removed from cache also Entry cachedUGI = cache.get(session); - if (expiredProxyUGI == cachedUGI) { + if (toDelete == cachedUGI) { // Remove it from cache, as cache now has an // expired entry which is not in progress cache.remove(session); @@ -177,42 +188,40 @@ private boolean closeUGI(Entry expiredProxyUGI) { // Optimization to call close only if it has not been // called for that UGI - if (!expiredProxyUGI.isCleaned()) { + if (!toDelete.isCleaned()) { LOG.info(session.toString() + " Closing " + fsMsg + " (Cache Size = " + cache.size() + ")"); - ugiProvider.destroy(expiredProxyUGI.getUGI()); - expiredProxyUGI.setCleaned(); + ugiProvider.destroy(toDelete.getUGI()); + toDelete.setCleaned(); } } catch (Throwable t) { - LOG.warn(session.toString() + " Error closing " + fsMsg); + LOG.warn(session.toString() + " Error closing " + fsMsg, t); } return true; } /** - * Stores a {@link UserGroupInformation} associated with a {@link SessionId}, and determines - * when to expire the UGI. + * Stores a {@link UserGroupInformation}, and determines when to expire the UGI. */ - public static class Entry implements Delayed { + private static class Entry implements Delayed { private volatile long startTime; private UserGroupInformation proxyUGI; - private SessionId session; private boolean cleaned = false; private AtomicInteger referenceCount = new AtomicInteger(); + private final Ticker ticker; private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes /** * Creates a new UGICache Entry. * + * @param ticker * @param proxyUGI - * @param session */ - Entry(UserGroupInformation proxyUGI, SessionId session) { - this.startTime = System.currentTimeMillis(); + Entry(Ticker ticker, UserGroupInformation proxyUGI) { + this.ticker = ticker; this.proxyUGI = proxyUGI; - this.session = session; } /** @@ -222,13 +231,6 @@ public UserGroupInformation getUGI() { return proxyUGI; } - /** - * @return the {@link SessionId}. - */ - SessionId getSession() { - return session; - } - /** * @return true if the setCleaned has been invoked, false otherwise. */ @@ -244,10 +246,10 @@ void setCleaned() { } /** - * @return the number of active requests using the {@link UserGroupInformation}. + * @return true if the UGI is being referenced by a session, false otherwise */ - int countReferences() { - return referenceCount.get(); + private boolean isInUse() { + return referenceCount.get() > 0; } /** @@ -261,14 +263,15 @@ void acquireReference() { * Decrements the number of references accessing the {@link UserGroupInformation}. */ void releaseReference() { - referenceCount.decrementAndGet(); + int count = referenceCount.decrementAndGet(); + assert count >= 0; } /** * Resets the timer for removing this Entry from the cache. */ void resetTime() { - startTime = System.currentTimeMillis(); + startTime = currentTimeMillis(); } /** @@ -302,7 +305,14 @@ public int compareTo(Delayed other) { * @return the number of milliseconds remaining before this cache entry expires. */ private long getDelayMillis() { - return (startTime + UGI_CACHE_EXPIRY) - System.currentTimeMillis(); + return (startTime + UGI_CACHE_EXPIRY) - currentTimeMillis(); + } + + /** + * @return the current Unix timestamp in milliseconds (equivalent to {@link System}.currentTimeMillis) + */ + private long currentTimeMillis() { + return ticker.read() / 1000; } } } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index d4265ca7c4..3f14432184 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -70,9 +70,9 @@ public void init(FilterConfig filterConfig) throws ServletException { * and create a proxy user to execute further request chain. Responds with an HTTP error if the header is missing * or the chain processing throws an exception. * - * @param request http request + * @param request http request * @param response http response - * @param chain filter chain + * @param chain filter chain */ @Override public void doFilter(final ServletRequest request, final ServletResponse response, final FilterChain chain) @@ -105,9 +105,9 @@ 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 - UGICache.Entry timedProxyUGI = cache.getTimedProxyUGI(session); + UserGroupInformation ugi = cache.getUserGroupInformation(session); try { - timedProxyUGI.getUGI().doAs(action); + ugi.doAs(action); } catch (UndeclaredThrowableException ute) { // unwrap the real exception thrown by the action throw new ServletException(ute.getCause()); @@ -116,7 +116,7 @@ public Boolean run() throws IOException, ServletException { } finally { // Optimization to cleanup the cache if it is the last fragment boolean forceClean = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); - cache.release(timedProxyUGI, forceClean); + cache.release(session, forceClean); } } else { // no user impersonation is configured @@ -124,7 +124,6 @@ public Boolean run() throws IOException, ServletException { } } - private Integer getHeaderValueInt(ServletRequest request, String headerKey, boolean required) throws IllegalArgumentException { String value = getHeaderValue(request, headerKey, required); diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index ca3dff250b..9f7576ea75 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -19,16 +19,19 @@ * under the License. */ +import com.google.common.base.Ticker; import org.apache.hadoop.security.UserGroupInformation; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; +import java.util.concurrent.atomic.AtomicLong; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -37,9 +40,24 @@ import static org.powermock.api.mockito.PowerMockito.when; public class UGICacheTest { + private static final long MINUTES = 60 * 1000L; private UGIProvider provider = null; private SessionId session = null; private UGICache cache = null; + private FakeTicker fakeTicker; + + private static class FakeTicker extends Ticker { + private final AtomicLong nanos = new AtomicLong(); + + @Override + public long read() { + return nanos.get(); + } + + public long advanceTime(long milliseconds) { + return nanos.addAndGet(milliseconds * 1000) / 1000; + } + } @Before public void setUp() throws Exception { @@ -52,30 +70,30 @@ public UserGroupInformation answer(InvocationOnMock invocation) { }); session = new SessionId(0, "txn-id", "the-user"); - cache = new UGICache(provider); + fakeTicker = new FakeTicker(); + cache = new UGICache(provider, fakeTicker); } @Test public void getUGIFromEmptyCache() throws Exception { - UGICache.Entry entry = cache.getTimedProxyUGI(session); - assertNotNull(entry.getUGI()); + UserGroupInformation ugi = cache.getUserGroupInformation(session); + assertNotNull(ugi); verify(provider).createProxyUGI("the-user"); } @Test public void getSameUGITwiceUsesCache() throws Exception { - UGICache.Entry entry1 = cache.getTimedProxyUGI(session); - UGICache.Entry entry2 = cache.getTimedProxyUGI(session); - assertEquals(entry1, entry2); - assertNotNull(entry1.getUGI()); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + assertEquals(ugi1, ugi2); verify(provider, times(1)).createProxyUGI("the-user"); } @Test public void getTwoUGIsWithDifferentSessionsForSameUser() throws Exception { SessionId otherSession = new SessionId(0, "txn-id-2", "the-user"); - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(otherSession); + UserGroupInformation proxyUGI1 = cache.getUserGroupInformation(session); + UserGroupInformation proxyUGI2 = cache.getUserGroupInformation(otherSession); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(2)).createProxyUGI("the-user"); // TODO: this seems weird. We're creating two UGIs with the same params, @@ -85,20 +103,20 @@ public void getTwoUGIsWithDifferentSessionsForSameUser() throws Exception { @Test public void getTwoUGIsWithDifferentUsers() throws Exception { SessionId otherSession = new SessionId(0, "txn-id", "different-user"); - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(otherSession); + UserGroupInformation proxyUGI1 = cache.getUserGroupInformation(session); + UserGroupInformation proxyUGI2 = cache.getUserGroupInformation(otherSession); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(1)).createProxyUGI("the-user"); verify(provider, times(1)).createProxyUGI("different-user"); } @Test - public void getTwoUGIsWithDifferentUsersCachesBoth() throws Exception { + public void gettingTwoUGIsWithDifferentUsersCachesBoth() throws Exception { SessionId otherSession = new SessionId(0, "txn-id", "different-user"); - UGICache.Entry proxyUGI1a = cache.getTimedProxyUGI(session); - UGICache.Entry proxyUGI1b = cache.getTimedProxyUGI(session); - UGICache.Entry proxyUGI2a = cache.getTimedProxyUGI(otherSession); - UGICache.Entry proxyUGI2b = cache.getTimedProxyUGI(otherSession); + UserGroupInformation proxyUGI1a = cache.getUserGroupInformation(session); + UserGroupInformation proxyUGI1b = cache.getUserGroupInformation(session); + UserGroupInformation proxyUGI2a = cache.getUserGroupInformation(otherSession); + UserGroupInformation proxyUGI2b = cache.getUserGroupInformation(otherSession); assertEquals(proxyUGI1a, proxyUGI1b); assertEquals(proxyUGI2a, proxyUGI2b); assertNotEquals(proxyUGI1a, proxyUGI2a); @@ -107,131 +125,131 @@ public void getTwoUGIsWithDifferentUsersCachesBoth() throws Exception { } @Test + @Ignore public void getUGIWhenRequestedUserDoesNotExist() throws Exception { - // what does UserGroupInformation.createProxyUser() do in this scenario? + // TODO: what does UserGroupInformation.createProxyUser() do in this scenario? // how about getLoginUser()? } @Test public void anySegmentIdIsValid() throws Exception { session = new SessionId(65, "txn-id", "the-user"); - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); - assertNotNull(proxyUGI1.getUGI()); + UserGroupInformation proxyUGI1 = cache.getUserGroupInformation(session); + assertNotNull(proxyUGI1); } @Test public void releaseWithoutForceClean() throws Exception { - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - cache.release(proxyUGI1, false); + cache.release(session, false); // UGI wasn't cleaned up, so we can still get it - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); - assertEquals(proxyUGI1, proxyUGI2); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + assertEquals(ugi1, ugi2); verify(provider, times(1)).createProxyUGI("the-user"); } @Test public void releaseWithForceClean() throws Exception { - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - cache.release(proxyUGI1, true); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); - assertNotEquals(proxyUGI1, proxyUGI2); + cache.release(session, true); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + assertNotEquals(ugi1, ugi2); verify(provider, times(2)).createProxyUGI("the-user"); } @Test - public void releaseWithForceCleanResetsTheExpirationTimeIfUGIIsReferenced() throws Exception { - UGICache.Entry reference1 = cache.getTimedProxyUGI(session); - UGICache.Entry reference2 = cache.getTimedProxyUGI(session); + public void releaseResetsTheExpirationTime() throws Exception { + UserGroupInformation reference1 = cache.getUserGroupInformation(session); + UserGroupInformation reference2 = cache.getUserGroupInformation(session); - cache.release(reference1, true); - cache.release(reference2, false); + cache.release(session, true); + fakeTicker.advanceTime(15 * MINUTES); + cache.release(session, false); + fakeTicker.advanceTime(10 * MINUTES); - UGICache.Entry reference3 = cache.getTimedProxyUGI(session); + UserGroupInformation reference3 = cache.getUserGroupInformation(session); + assertEquals(reference1, reference2); assertEquals(reference1, reference3); + verify(provider, never()).destroy(any(UserGroupInformation.class)); } @Test public void releaseAndReacquireDoesNotFreeResources() throws Exception { - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); - - cache.release(proxyUGI1, false); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); - proxyUGI2.setExpired(); - UGICache.Entry proxyUGI3 = cache.getTimedProxyUGI(session); - // this does not clean up any UGIs because our proxyUGI is still in use. - assertEquals(proxyUGI3, proxyUGI2); + cache.getUserGroupInformation(session); + cache.release(session, false); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + fakeTicker.advanceTime(50 * MINUTES); + UserGroupInformation ugi3 = cache.getUserGroupInformation(session); + // this does not clean up any UGIs because our ugi is still in use. + assertEquals(ugi3, ugi2); verify(provider, times(1)).createProxyUGI("the-user"); verify(provider, never()).destroy(any(UserGroupInformation.class)); } @Test public void releaseAndAcquireAfterTimeoutFreesResources() throws Exception { - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - cache.release(proxyUGI1, false); - proxyUGI1.setExpired(); + cache.release(session, false); + fakeTicker.advanceTime(20 * MINUTES); verify(provider, never()).destroy(any(UserGroupInformation.class)); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); - verify(provider).destroy(proxyUGI1.getUGI()); - assertNotEquals(proxyUGI2, proxyUGI1); - assertNotEquals(proxyUGI2.getUGI(), proxyUGI1.getUGI()); - verify(provider, never()).destroy(proxyUGI2.getUGI()); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + verify(provider).destroy(ugi1); + assertNotEquals(ugi2, ugi1); + verify(provider, never()).destroy(ugi2); // this does not clean up any UGIs because our proxyUGI is still in use. - assertNotEquals(proxyUGI2, proxyUGI1); verify(provider, times(2)).createProxyUGI("the-user"); } @Test public void releaseAnExpiredUGIResetsTheTimer() throws Exception { - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); - proxyUGI1.setExpired(); - cache.release(proxyUGI1, false); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); - assertEquals(proxyUGI2, proxyUGI1); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + fakeTicker.advanceTime(20 * MINUTES); + cache.release(session, false); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + assertEquals(ugi2, ugi1); verify(provider, never()).destroy(any(UserGroupInformation.class)); } @Test public void releaseDoesNotFreeResourcesIfUGIIsUsedElsewhere() throws Exception { - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); - cache.release(proxyUGI1, true); + cache.release(session, true); + fakeTicker.advanceTime(60 * MINUTES); // UGI was not cleaned up because proxyUGI2 is referencing it - UGICache.Entry proxyUGI3 = cache.getTimedProxyUGI(session); - assertEquals(proxyUGI1, proxyUGI2); - assertEquals(proxyUGI1, proxyUGI3); + UserGroupInformation ugi3 = cache.getUserGroupInformation(session); + assertEquals(ugi1, ugi2); + assertEquals(ugi1, ugi3); verify(provider, times(1)).createProxyUGI("the-user"); } @Test public void releasingAllReferencesFreesResources() throws Exception { - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + + cache.release(session, true); + verify(provider, never()).destroy(any(UserGroupInformation.class)); + cache.release(session, true); + verify(provider, times(1)).destroy(any(UserGroupInformation.class)); - cache.release(proxyUGI1, true); - cache.release(proxyUGI2, true); // at this point, the initial UGI has been freed. Calling - // getTimedProxyUGI again creates a new UGI. - UGICache.Entry proxyUGI3 = cache.getTimedProxyUGI(session); - assertEquals(proxyUGI1, proxyUGI2); - assertNotEquals(proxyUGI1, proxyUGI3); + // getUserGroupInformation again creates a new UGI. + UserGroupInformation proxyUGI3 = cache.getUserGroupInformation(session); + assertEquals(ugi1, ugi2); + assertNotEquals(ugi1, proxyUGI3); verify(provider, times(2)).createProxyUGI("the-user"); } @Test - public void releaseAnEntryNotInTheCache() throws Exception { - // this could happen if some caller of the cache - // retains a reference to a cache entry after releasing it. - UGICache.Entry proxyUGI1 = cache.getTimedProxyUGI(session); + public void releaseAnEntryNotInTheCache() { + // this could happen if some caller of the cache calls release twice. - cache.release(proxyUGI1, true); - cache.release(proxyUGI1, true); - UGICache.Entry proxyUGI2 = cache.getTimedProxyUGI(session); - assertNotEquals(proxyUGI1, proxyUGI2); - verify(provider, times(2)).createProxyUGI("the-user"); + cache.release(session, false); // does not throw } } From 7f2bc0c4e6debfb58d89e37eb2bc331bb8797d38 Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Wed, 11 Jul 2018 17:52:06 -0700 Subject: [PATCH 16/28] Refactored tests addressing PR comments. Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 4 +- .../apache/hawq/pxf/service/UGICacheTest.java | 83 ++++++++++--------- 2 files changed, 48 insertions(+), 39 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 5d523cdbe1..e0d6930831 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -49,6 +49,7 @@ public class UGICache { private final Map> queueMap = new HashMap<>(); private final UGIProvider ugiProvider; private Ticker ticker; + private final static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes /** * Create a UGICache with the given {@link UGIProvider}. Intended for use by tests which need @@ -209,9 +210,8 @@ private static class Entry implements Delayed { private volatile long startTime; private UserGroupInformation proxyUGI; private boolean cleaned = false; - private AtomicInteger referenceCount = new AtomicInteger(); + private final AtomicInteger referenceCount = new AtomicInteger(); private final Ticker ticker; - private static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes /** * Creates a new UGICache Entry. diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index 9f7576ea75..f6155b27e2 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -32,6 +32,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -54,7 +56,7 @@ public long read() { return nanos.get(); } - public long advanceTime(long milliseconds) { + long advanceTime(long milliseconds) { return nanos.addAndGet(milliseconds * 1000) / 1000; } } @@ -113,39 +115,42 @@ public void getTwoUGIsWithDifferentUsers() throws Exception { @Test public void gettingTwoUGIsWithDifferentUsersCachesBoth() throws Exception { SessionId otherSession = new SessionId(0, "txn-id", "different-user"); - UserGroupInformation proxyUGI1a = cache.getUserGroupInformation(session); - UserGroupInformation proxyUGI1b = cache.getUserGroupInformation(session); - UserGroupInformation proxyUGI2a = cache.getUserGroupInformation(otherSession); - UserGroupInformation proxyUGI2b = cache.getUserGroupInformation(otherSession); - assertEquals(proxyUGI1a, proxyUGI1b); - assertEquals(proxyUGI2a, proxyUGI2b); - assertNotEquals(proxyUGI1a, proxyUGI2a); + UserGroupInformation ugi1RefA = cache.getUserGroupInformation(session); + UserGroupInformation ugi1RefB = cache.getUserGroupInformation(session); + UserGroupInformation ugi2RefA = cache.getUserGroupInformation(otherSession); + UserGroupInformation ugi2RefB = cache.getUserGroupInformation(otherSession); + assertSame(ugi1RefA, ugi1RefB); + assertSame(ugi2RefA, ugi2RefB); + assertNotEquals(ugi1RefA, ugi2RefA); verify(provider, times(1)).createProxyUGI("the-user"); verify(provider, times(1)).createProxyUGI("different-user"); } - @Test - @Ignore - public void getUGIWhenRequestedUserDoesNotExist() throws Exception { - // TODO: what does UserGroupInformation.createProxyUser() do in this scenario? - // how about getLoginUser()? - } - @Test public void anySegmentIdIsValid() throws Exception { - session = new SessionId(65, "txn-id", "the-user"); + int crazySegId = Integer.MAX_VALUE; + session = new SessionId(crazySegId, "txn-id", "the-user"); UserGroupInformation proxyUGI1 = cache.getUserGroupInformation(session); assertNotNull(proxyUGI1); } + @Test + public void ensureCleanUpAfterExpiration() throws Exception { + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + + fakeTicker.advanceTime(10 * MINUTES); + cache.release(session, false); + fakeTicker.advanceTime(16 * MINUTES); + assertNotInCache(session, ugi1); + } + @Test public void releaseWithoutForceClean() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); cache.release(session, false); // UGI wasn't cleaned up, so we can still get it - UserGroupInformation ugi2 = cache.getUserGroupInformation(session); - assertEquals(ugi1, ugi2); + assertStillInCache(session, ugi1); verify(provider, times(1)).createProxyUGI("the-user"); } @@ -154,26 +159,20 @@ public void releaseWithForceClean() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); cache.release(session, true); - UserGroupInformation ugi2 = cache.getUserGroupInformation(session); - assertNotEquals(ugi1, ugi2); - verify(provider, times(2)).createProxyUGI("the-user"); + assertNotInCache(session, ugi1); } @Test public void releaseResetsTheExpirationTime() throws Exception { UserGroupInformation reference1 = cache.getUserGroupInformation(session); - UserGroupInformation reference2 = cache.getUserGroupInformation(session); + cache.getUserGroupInformation(session); cache.release(session, true); fakeTicker.advanceTime(15 * MINUTES); cache.release(session, false); fakeTicker.advanceTime(10 * MINUTES); - UserGroupInformation reference3 = cache.getUserGroupInformation(session); - - assertEquals(reference1, reference2); - assertEquals(reference1, reference3); - verify(provider, never()).destroy(any(UserGroupInformation.class)); + assertStillInCache(session, reference1); } @Test @@ -217,15 +216,12 @@ public void releaseAnExpiredUGIResetsTheTimer() throws Exception { @Test public void releaseDoesNotFreeResourcesIfUGIIsUsedElsewhere() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + cache.getUserGroupInformation(session); cache.release(session, true); fakeTicker.advanceTime(60 * MINUTES); - // UGI was not cleaned up because proxyUGI2 is referencing it - UserGroupInformation ugi3 = cache.getUserGroupInformation(session); - assertEquals(ugi1, ugi2); - assertEquals(ugi1, ugi3); - verify(provider, times(1)).createProxyUGI("the-user"); + // UGI was not cleaned up because we are still holding a reference + assertStillInCache(session, ugi1); } @Test @@ -233,6 +229,8 @@ public void releasingAllReferencesFreesResources() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); UserGroupInformation ugi2 = cache.getUserGroupInformation(session); + assertEquals(ugi1, ugi2); + cache.release(session, true); verify(provider, never()).destroy(any(UserGroupInformation.class)); cache.release(session, true); @@ -240,10 +238,7 @@ public void releasingAllReferencesFreesResources() throws Exception { // at this point, the initial UGI has been freed. Calling // getUserGroupInformation again creates a new UGI. - UserGroupInformation proxyUGI3 = cache.getUserGroupInformation(session); - assertEquals(ugi1, ugi2); - assertNotEquals(ugi1, proxyUGI3); - verify(provider, times(2)).createProxyUGI("the-user"); + assertNotInCache(session, ugi1); } @Test @@ -252,4 +247,18 @@ public void releaseAnEntryNotInTheCache() { cache.release(session, false); // does not throw } + + private void assertStillInCache(SessionId session, UserGroupInformation ugi) throws Exception { + UserGroupInformation fromCache = cache.getUserGroupInformation(session); + assertSame(ugi, fromCache); + cache.release(session, false); + verify(provider, never()).destroy(ugi); + } + + private void assertNotInCache(SessionId session, UserGroupInformation ugi) throws Exception { + UserGroupInformation fromCache = cache.getUserGroupInformation(session); + assertNotSame(ugi, fromCache); + cache.release(session, false); + verify(provider, times(1)).destroy(ugi); + } } From 3582ba3be59ef9e2d0434d3886697b0c376d9cac Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Thu, 12 Jul 2018 13:48:38 -0700 Subject: [PATCH 17/28] Address PR comments - Use UGI_CACHE_EXPIRY in tests and comments - Change null check to assert to express that cache entries should not be double-freed by correctly-functioning client code. - Add tests for exceptional cases Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 10 ++++---- .../src/main/resources/pxf-log4j.properties | 4 ++-- .../pxf/service/UGICacheMultiThreadTest.java | 4 ++++ .../apache/hawq/pxf/service/UGICacheTest.java | 23 +++++++++++++++---- 4 files changed, 29 insertions(+), 12 deletions(-) create mode 100644 pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index e0d6930831..b794e1dee9 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -35,7 +35,7 @@ /** * Stores UserGroupInformation instances for each active session. The UGIs are cleaned up if they - * have not been accessed for 15 minutes. + * have not been accessed for UGI_CACHE_EXPIRY milliseconds. *

* The motivation for caching is that destroying UGIs is slow. The alternative, creating and * destroying a UGI per-request, is wasteful. @@ -49,7 +49,7 @@ public class UGICache { private final Map> queueMap = new HashMap<>(); private final UGIProvider ugiProvider; private Ticker ticker; - private final static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes + final static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes /** * Create a UGICache with the given {@link UGIProvider}. Intended for use by tests which need @@ -92,7 +92,7 @@ public UserGroupInformation getUserGroupInformation(SessionId session) throws IO /** * Decrement reference count for the given session's UGI. Resets the time at which the UGI will - * expire to 15 minutes in the future. + * expire to UGI_CACHE_EXPIRY milliseconds in the future. * * @param session the session for which we want to release the UGI. * @param cleanImmediatelyIfNoRefs if true, destroys the UGI for the given session (only if it is @@ -102,7 +102,7 @@ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { Entry timedProxyUGI = cache.get(session); - if (timedProxyUGI == null) return; + assert timedProxyUGI != null : "Cannot release UGI for this session; it is not cached: " + session; timedProxyUGI.resetTime(); timedProxyUGI.releaseReference(); @@ -170,7 +170,7 @@ private void cleanup(SessionId session) { private boolean closeUGI(SessionId session, Entry toDelete) { // There is no need to synchronize this method because it should be called // from within a synchronized block - String fsMsg = "FileSystem for proxy user = " + toDelete.getUGI().getUserName(); + String fsMsg = "FileSystem for proxy user = " + session.getUser(); try { // The UGI object is still being used by another thread if (toDelete.isInUse()) { diff --git a/pxf/pxf-service/src/main/resources/pxf-log4j.properties b/pxf/pxf-service/src/main/resources/pxf-log4j.properties index f6bb0eaf74..3b64e8ac0d 100644 --- a/pxf/pxf-service/src/main/resources/pxf-log4j.properties +++ b/pxf/pxf-service/src/main/resources/pxf-log4j.properties @@ -5,9 +5,9 @@ # to you under the Apache License, Version 2.0 (the # "License"); you may not use this file except in compliance # with the License. You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, # software distributed under the License is distributed on an # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java new file mode 100644 index 0000000000..9e11a37ad4 --- /dev/null +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java @@ -0,0 +1,4 @@ +package org.apache.hawq.pxf.service; + +public class UGICacheMultiThreadTest { +} diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index f6155b27e2..4cd3cbf116 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -22,11 +22,11 @@ import com.google.common.base.Ticker; import org.apache.hadoop.security.UserGroupInformation; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; +import java.io.IOException; import java.util.concurrent.atomic.AtomicLong; import static org.junit.Assert.assertEquals; @@ -35,6 +35,7 @@ import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -48,7 +49,7 @@ public class UGICacheTest { private UGICache cache = null; private FakeTicker fakeTicker; - private static class FakeTicker extends Ticker { + static class FakeTicker extends Ticker { private final AtomicLong nanos = new AtomicLong(); @Override @@ -168,7 +169,7 @@ public void releaseResetsTheExpirationTime() throws Exception { cache.getUserGroupInformation(session); cache.release(session, true); - fakeTicker.advanceTime(15 * MINUTES); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY); cache.release(session, false); fakeTicker.advanceTime(10 * MINUTES); @@ -241,11 +242,23 @@ public void releasingAllReferencesFreesResources() throws Exception { assertNotInCache(session, ugi1); } + @Test(expected = IOException.class) + public void errorsThrownByCreatingAUgiAreNotCaught() throws Exception { + when(provider.createProxyUGI("the-user")).thenThrow(new IOException("test exception")); + cache.getUserGroupInformation(session); + } + @Test + public void errorsThrownByDestroyingAUgiAreCaught() throws Exception { + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + doThrow(new IOException("test exception")).when(provider).destroy(ugi1); + cache.release(session, true); // does not throw + } + + @Test(expected = AssertionError.class) public void releaseAnEntryNotInTheCache() { // this could happen if some caller of the cache calls release twice. - - cache.release(session, false); // does not throw + cache.release(session, false); } private void assertStillInCache(SessionId session, UserGroupInformation ugi) throws Exception { From e820a430504aeb731eb6900e01682a2c820584b1 Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Thu, 12 Jul 2018 13:51:08 -0700 Subject: [PATCH 18/28] Test synchronization of UGICache using multiple threads Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../pxf/service/UGICacheMultiThreadTest.java | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java index 9e11a37ad4..b3a4f7bcc0 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java @@ -1,4 +1,127 @@ package org.apache.hawq.pxf.service; +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import io.netty.util.internal.ConcurrentSet; +import org.apache.hadoop.security.UserGroupInformation; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; +import java.security.SecureRandom; +import java.util.Random; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; + public class UGICacheMultiThreadTest { + private FakeUgiProvider provider = null; + private static final int numberOfSegments = 3; + private static final int numberOfUsers = 3; + private static final int numberOfTxns = 3; + private SessionId[] sessions = new SessionId[numberOfSegments * numberOfUsers * numberOfTxns]; + private UGICache cache = null; + private UGICacheTest.FakeTicker fakeTicker; + + class FakeUgiProvider extends UGIProvider { + Set ugis = new ConcurrentSet<>(); + + @Override + UserGroupInformation createProxyUGI(String effectiveUser) { + UserGroupInformation ugi = mock(UserGroupInformation.class); + ugis.add(ugi); + return ugi; + } + + @Override + void destroy(UserGroupInformation ugi) { + if (!ugis.remove(ugi)) { + throw new IllegalStateException("Tried to destroy UGI that does not exist"); + } + } + + int countUgisInUse() { + return ugis.size(); + } + } + + @Before + public void setUp() { + provider = new FakeUgiProvider(); + + int l = 0; + for (int i = 0; i < numberOfSegments; i++) { + for (int j = 0; j < numberOfUsers; j++) { + for (int k = 0; k < numberOfTxns; k++) { + sessions[l++] = new SessionId(i, "txn-id-" + k, "the-user-" + j); + } + } + } + fakeTicker = new UGICacheTest.FakeTicker(); + cache = new UGICache(provider, fakeTicker); + } + + @Test + public void multiThreadedTest() throws Exception { + final Random rnd = new SecureRandom(); + final AtomicInteger finishedCount = new AtomicInteger(); + + int threadCount = 500; + Thread[] threads = new Thread[threadCount]; + + for (int i = 0; i < threads.length; i++) { + threads[i] = new Thread(new Runnable() { + @Override + public void run() { + try { + for (int i = 0; i < 100; i++) { + for (SessionId session : sessions) { + cache.getUserGroupInformation(session); + } + Thread.sleep(0, rnd.nextInt(1000)); + for (SessionId session : sessions) { + cache.release(session, false); + } + } + + for (SessionId session : sessions) { + cache.getUserGroupInformation(session); + cache.release(session, true); + } + + finishedCount.incrementAndGet(); + } catch (IOException | InterruptedException e) { + e.printStackTrace(); + } + } + }); + threads[i].start(); + } + + for (Thread thread : threads) { + thread.join(); + } + + assertEquals(threadCount, finishedCount.intValue()); + assertEquals(0, provider.countUgisInUse()); + } } From 15704a31f5b48ddaa376bc3f6078bf78b586d29d Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Thu, 12 Jul 2018 17:15:54 -0700 Subject: [PATCH 19/28] Address PR Comments and add missing tests. - Fixed an issue when releasing an entry at the head of the queue, we reset the expirationTime, and subsequent expired elements in the queue would not be closed. Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../apache/hawq/pxf/service/SessionId.java | 6 +- .../org/apache/hawq/pxf/service/UGICache.java | 101 +++++++++--------- .../servlet/SecurityServletFilter.java | 17 +-- .../apache/hawq/pxf/service/UGICacheTest.java | 35 +++++- 4 files changed, 94 insertions(+), 65 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java index f0f3e9b232..3829f21836 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java @@ -19,8 +19,6 @@ * under the License. */ -import org.apache.commons.lang3.builder.EqualsBuilder; - /** * For the purposes of pxf-server, a session is the set of requests processed on a specific segment * on behalf of a particular user and transaction. Grouping requests together into a session allows @@ -80,9 +78,7 @@ public boolean equals(Object obj) { if (obj.getClass() != getClass()) return false; SessionId that = (SessionId) obj; - return new EqualsBuilder() - .append(sessionId, that.sessionId) - .isEquals(); + return this.sessionId.equals(that.sessionId); } /** diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index b794e1dee9..7975509a48 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -44,9 +44,8 @@ public class UGICache { private static final Log LOG = LogFactory.getLog(UGICache.class); private Map cache = new ConcurrentHashMap<>(); - @SuppressWarnings("unchecked") // There is a separate DelayQueue for each segment (also being used for locking) - private final Map> queueMap = new HashMap<>(); + private final Map> expirationQueueMap = new HashMap<>(); private final UGIProvider ugiProvider; private Ticker ticker; final static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes @@ -80,12 +79,12 @@ public UserGroupInformation getUserGroupInformation(SessionId session) throws IO cleanup(session); Entry entry = cache.get(session); if (entry == null) { - LOG.info(session.toString() + " Creating proxy user = " + user); + LOG.debug(session.toString() + " Creating proxy user = " + user); entry = new Entry(ticker, ugiProvider.createProxyUGI(user)); delayQueue.offer(entry); cache.put(session, entry); } - entry.acquireReference(); + entry.incrementRefCount(); return entry.getUGI(); } } @@ -100,35 +99,40 @@ public UserGroupInformation getUserGroupInformation(SessionId session) throws IO */ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { - Entry timedProxyUGI = cache.get(session); + Entry entry = cache.get(session); - assert timedProxyUGI != null : "Cannot release UGI for this session; it is not cached: " + session; + if (entry == null) { + throw new IllegalStateException("Cannot release UGI for this session; it is not cached: " + session); + } + + DelayQueue expirationQueue = getExpirationQueue(session.getSegmentId()); - timedProxyUGI.resetTime(); - timedProxyUGI.releaseReference(); - if (cleanImmediatelyIfNoRefs) { - synchronized (getExpirationQueue(session.getSegmentId())) { - timedProxyUGI.setExpired(); - closeUGI(session, timedProxyUGI); + synchronized (expirationQueue) { + expirationQueue.remove(entry); + entry.resetTime(); + expirationQueue.offer(entry); + entry.decrementRefCount(); + if (cleanImmediatelyIfNoRefs && entry.isNotInUse()) { + closeUGI(session, entry); } } } /** * Get the queue of cache entries associated with a segment, creating it if it doesn't - * yet exist. This lets us lazily populate the queueMap. + * yet exist. This lets us lazily populate the expirationQueueMap. * * @param segmentId * @return the {@link DelayQueue} associated to the segment. */ private DelayQueue getExpirationQueue(Integer segmentId) { - DelayQueue queue = queueMap.get(segmentId); + DelayQueue queue = expirationQueueMap.get(segmentId); if (queue == null) { - synchronized (queueMap) { - queue = queueMap.get(segmentId); + synchronized (expirationQueueMap) { + queue = expirationQueueMap.get(segmentId); if (queue == null) { queue = new DelayQueue<>(); - queueMap.put(segmentId, queue); + expirationQueueMap.put(segmentId, queue); } } } @@ -144,16 +148,21 @@ private DelayQueue getExpirationQueue(Integer segmentId) { */ private void cleanup(SessionId session) { - Entry ugi; - DelayQueue delayQueue = getExpirationQueue(session.getSegmentId()); - while ((ugi = delayQueue.poll()) != null) { - // Place it back in the queue if still in use and was not closed - if (!closeUGI(session, ugi)) { - ugi.resetTime(); - delayQueue.offer(ugi); + Entry expiredUgi; + DelayQueue expirationQueue = getExpirationQueue(session.getSegmentId()); + while ((expiredUgi = expirationQueue.poll()) != null) { + if (expiredUgi.isNotInUse()) { + closeUGI(session, expiredUgi); + } else { + // The UGI object is still being used by another thread + String fsMsg = "FileSystem for proxy user = " + session.getUser(); + LOG.debug(session.toString() + " Skipping close of " + fsMsg); + // Place it back in the queue if still in use and was not closed + expiredUgi.resetTime(); + expirationQueue.offer(expiredUgi); } LOG.debug("Delay Queue Size for segment " + - session.getSegmentId() + " = " + delayQueue.size()); + session.getSegmentId() + " = " + expirationQueue.size()); } } @@ -167,30 +176,20 @@ private void cleanup(SessionId session) { * @return true if the UGI entry was cleaned, false when the UGI entry was still in use * and cleaning up was skipped */ - private boolean closeUGI(SessionId session, Entry toDelete) { + private void closeUGI(SessionId session, Entry toDelete) { // There is no need to synchronize this method because it should be called // from within a synchronized block String fsMsg = "FileSystem for proxy user = " + session.getUser(); try { - // The UGI object is still being used by another thread - if (toDelete.isInUse()) { - LOG.info(session.toString() + " Skipping close of " + fsMsg); - return false; - } - // Expired UGI object can be cleaned since it is not used - // Determine if it can be removed from cache also - Entry cachedUGI = cache.get(session); - if (toDelete == cachedUGI) { - // Remove it from cache, as cache now has an - // expired entry which is not in progress - cache.remove(session); - } + // Remove it from cache, as cache now has an + // expired entry which is not in progress + cache.remove(session); // Optimization to call close only if it has not been // called for that UGI if (!toDelete.isCleaned()) { - LOG.info(session.toString() + " Closing " + fsMsg + + LOG.debug(session.toString() + " Closing " + fsMsg + " (Cache Size = " + cache.size() + ")"); ugiProvider.destroy(toDelete.getUGI()); toDelete.setCleaned(); @@ -199,7 +198,6 @@ private boolean closeUGI(SessionId session, Entry toDelete) { } catch (Throwable t) { LOG.warn(session.toString() + " Error closing " + fsMsg, t); } - return true; } /** @@ -248,23 +246,25 @@ void setCleaned() { /** * @return true if the UGI is being referenced by a session, false otherwise */ - private boolean isInUse() { - return referenceCount.get() > 0; + private boolean isNotInUse() { + return referenceCount.get() <= 0; } /** * Increments the number of references accessing the {@link UserGroupInformation}. */ - void acquireReference() { + void incrementRefCount() { referenceCount.incrementAndGet(); } /** * Decrements the number of references accessing the {@link UserGroupInformation}. */ - void releaseReference() { + void decrementRefCount() { int count = referenceCount.decrementAndGet(); - assert count >= 0; + if (count < 0) { + throw new IllegalStateException("UGICache.Entry referenceCount may not be decremented past 0."); + } } /** @@ -274,13 +274,6 @@ void resetTime() { startTime = currentTimeMillis(); } - /** - * Expire the UGICache Entry - */ - void setExpired() { - startTime = -1L; - } - /** * {@inheritDoc} */ @@ -297,6 +290,8 @@ public long getDelay(TimeUnit unit) { */ @Override public int compareTo(Delayed other) { + if (!(other instanceof Entry)) return 1; + Entry that = (Entry) other; return Long.compare(this.getDelayMillis(), that.getDelayMillis()); } diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 3f14432184..32ae09a24d 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -53,7 +53,7 @@ public class SecurityServletFilter implements Filter { private static final String FRAGMENT_COUNT_HEADER = "X-GP-FRAGMENT-COUNT"; private static final String MISSING_HEADER_ERROR = "Header %s is missing in the request"; private static final String EMPTY_HEADER_ERROR = "Header %s is empty in the request"; - private static final UGICache cache = new UGICache(); + private static UGICache proxyUGICache; /** * Initializes the filter. @@ -62,7 +62,7 @@ public class SecurityServletFilter implements Filter { */ @Override public void init(FilterConfig filterConfig) throws ServletException { - //TODO: initialize cache here + proxyUGICache = new UGICache(); } /** @@ -105,9 +105,9 @@ 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 ugi = cache.getUserGroupInformation(session); + UserGroupInformation proxyUGI = proxyUGICache.getUserGroupInformation(session); try { - ugi.doAs(action); + proxyUGI.doAs(action); } catch (UndeclaredThrowableException ute) { // unwrap the real exception thrown by the action throw new ServletException(ute.getCause()); @@ -115,8 +115,13 @@ public Boolean run() throws IOException, ServletException { throw new ServletException(ie); } finally { // Optimization to cleanup the cache if it is the last fragment - boolean forceClean = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); - cache.release(session, forceClean); + boolean cleanImmediately = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); + try { + proxyUGICache.release(session, cleanImmediately); + } catch (Throwable t) { + LOG.error("Error releasing UGICache for session: " + session, t); + } + } } else { // no user impersonation is configured diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index 4cd3cbf116..2b20eb9db6 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -145,6 +145,39 @@ public void ensureCleanUpAfterExpiration() throws Exception { assertNotInCache(session, ugi1); } + @Test + public void anUnusedUGIIsFreedAfterDelayWhenAnotherUGIForTheSameSegmentIsAccessed() throws Exception { + SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); + UserGroupInformation notInUse = cache.getUserGroupInformation(session); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); + + // at this point, notInUse is expired but still in use + cache.getUserGroupInformation(session2); + cache.release(session, false); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); + + cache.getUserGroupInformation(session2); + + verify(provider, times(1)).destroy(notInUse); + } + + @Test + public void putsItemsBackInTheQueueWhenResettingExpirationDate() throws Exception { + SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); + SessionId session3 = new SessionId(0, "txn-id", "the-user-3"); + + cache.getUserGroupInformation(session); + fakeTicker.advanceTime(10 * MINUTES); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session2); + cache.release(session2, false); + fakeTicker.advanceTime(14 * MINUTES); + cache.release(session, false); + fakeTicker.advanceTime(2 * MINUTES); + cache.getUserGroupInformation(session3); + + verify(provider, times(1)).destroy(ugi2); + } + @Test public void releaseWithoutForceClean() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); @@ -255,7 +288,7 @@ public void errorsThrownByDestroyingAUgiAreCaught() throws Exception { cache.release(session, true); // does not throw } - @Test(expected = AssertionError.class) + @Test(expected = IllegalStateException.class) public void releaseAnEntryNotInTheCache() { // this could happen if some caller of the cache calls release twice. cache.release(session, false); From 25aeb353fa01707af6e2345f979bd2e66ac30b54 Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Fri, 13 Jul 2018 09:59:29 -0700 Subject: [PATCH 20/28] Addres PR Comments Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../apache/hawq/pxf/service/SessionId.java | 6 +- .../org/apache/hawq/pxf/service/UGICache.java | 59 +++++++++++-------- .../servlet/SecurityServletFilter.java | 16 ++--- .../apache/hawq/pxf/service/UGICacheTest.java | 1 + 4 files changed, 45 insertions(+), 37 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java index 3829f21836..172014b181 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/SessionId.java @@ -36,9 +36,9 @@ public class SessionId { /** * Create a sessionId * - * @param segmentId - * @param transactionId - * @param gpdbUser + * @param segmentId the calling segment + * @param transactionId the identifier for the transaction + * @param gpdbUser the GPDB username */ public SessionId(Integer segmentId, String transactionId, String gpdbUser) { this.segmentId = segmentId; diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 7975509a48..22fc6ba41d 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -43,11 +43,11 @@ public class UGICache { private static final Log LOG = LogFactory.getLog(UGICache.class); - private Map cache = new ConcurrentHashMap<>(); + private final Map cache = new ConcurrentHashMap<>(); // There is a separate DelayQueue for each segment (also being used for locking) private final Map> expirationQueueMap = new HashMap<>(); private final UGIProvider ugiProvider; - private Ticker ticker; + private final Ticker ticker; final static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes /** @@ -76,7 +76,7 @@ public UserGroupInformation getUserGroupInformation(SessionId session) throws IO DelayQueue delayQueue = getExpirationQueue(segmentId); synchronized (delayQueue) { // Use the opportunity to cleanup any expired entries - cleanup(session); + cleanup(delayQueue, session); Entry entry = cache.get(session); if (entry == null) { LOG.debug(session.toString() + " Creating proxy user = " + user); @@ -108,12 +108,15 @@ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { DelayQueue expirationQueue = getExpirationQueue(session.getSegmentId()); synchronized (expirationQueue) { - expirationQueue.remove(entry); - entry.resetTime(); - expirationQueue.offer(entry); entry.decrementRefCount(); + expirationQueue.remove(entry); if (cleanImmediatelyIfNoRefs && entry.isNotInUse()) { closeUGI(session, entry); + } else { + // Reset expiration time and put it back in the queue + // only when we don't close the UGI + entry.resetTime(); + expirationQueue.offer(entry); } } } @@ -140,29 +143,31 @@ private DelayQueue getExpirationQueue(Integer segmentId) { } /** - * Iterate through all the entries in the queue for the given session's segment - * and close expired {@link UserGroupInformation}, otherwise it resets - * the timer for every non-expired entry. + * Iterate through all the entries in the queue and close expired + * {@link UserGroupInformation}, otherwise it resets the timer for + * every non-expired entry. * + * @param expirationQueue * @param session */ - private void cleanup(SessionId session) { + private void cleanup(DelayQueue expirationQueue, SessionId session) { - Entry expiredUgi; - DelayQueue expirationQueue = getExpirationQueue(session.getSegmentId()); - while ((expiredUgi = expirationQueue.poll()) != null) { - if (expiredUgi.isNotInUse()) { - closeUGI(session, expiredUgi); + Entry expiredUGI; + while ((expiredUGI = expirationQueue.poll()) != null) { + if (expiredUGI.isNotInUse()) { + closeUGI(session, expiredUGI); } else { // The UGI object is still being used by another thread String fsMsg = "FileSystem for proxy user = " + session.getUser(); LOG.debug(session.toString() + " Skipping close of " + fsMsg); // Place it back in the queue if still in use and was not closed - expiredUgi.resetTime(); - expirationQueue.offer(expiredUgi); + expiredUGI.resetTime(); + expirationQueue.offer(expiredUGI); + } + if (LOG.isDebugEnabled()) { + LOG.debug("Delay Queue Size for segment " + + session.getSegmentId() + " = " + expirationQueue.size()); } - LOG.debug("Delay Queue Size for segment " + - session.getSegmentId() + " = " + expirationQueue.size()); } } @@ -172,11 +177,11 @@ private void cleanup(SessionId session) { * cache where it will then be processed to be destroyed by the UGIProvider. * * @param session - * @param toDelete + * @param expiredUGI * @return true if the UGI entry was cleaned, false when the UGI entry was still in use * and cleaning up was skipped */ - private void closeUGI(SessionId session, Entry toDelete) { + private void closeUGI(SessionId session, Entry expiredUGI) { // There is no need to synchronize this method because it should be called // from within a synchronized block String fsMsg = "FileSystem for proxy user = " + session.getUser(); @@ -188,11 +193,13 @@ private void closeUGI(SessionId session, Entry toDelete) { // Optimization to call close only if it has not been // called for that UGI - if (!toDelete.isCleaned()) { - LOG.debug(session.toString() + " Closing " + fsMsg + - " (Cache Size = " + cache.size() + ")"); - ugiProvider.destroy(toDelete.getUGI()); - toDelete.setCleaned(); + if (!expiredUGI.isCleaned()) { + if (LOG.isDebugEnabled()) { + LOG.debug(session.toString() + " Closing " + fsMsg + + " (Cache Size = " + cache.size() + ")"); + } + ugiProvider.destroy(expiredUGI.getUGI()); + expiredUGI.setCleaned(); } } catch (Throwable t) { diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 32ae09a24d..6c326fc33b 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -19,7 +19,6 @@ * under the License. */ - import java.io.IOException; import java.lang.reflect.UndeclaredThrowableException; import java.security.PrivilegedExceptionAction; @@ -34,12 +33,10 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.hadoop.security.UserGroupInformation; import org.apache.hawq.pxf.service.SessionId; import org.apache.hawq.pxf.service.UGICache; import org.apache.hawq.pxf.service.utilities.SecureLogin; - /** * Listener on lifecycle events of our webapp */ @@ -61,7 +58,7 @@ public class SecurityServletFilter implements Filter { * @param filterConfig filter configuration */ @Override - public void init(FilterConfig filterConfig) throws ServletException { + public void init(FilterConfig filterConfig) { proxyUGICache = new UGICache(); } @@ -104,10 +101,13 @@ 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 = proxyUGICache.getUserGroupInformation(session); + LOG.debug("Creating proxy user for session: " + session); try { - proxyUGI.doAs(action); + // create proxy user UGI from the UGI of the logged in user + // and execute the servlet chain as that user + proxyUGICache + .getUserGroupInformation(session) + .doAs(action); } catch (UndeclaredThrowableException ute) { // unwrap the real exception thrown by the action throw new ServletException(ute.getCause()); @@ -116,12 +116,12 @@ public Boolean run() throws IOException, ServletException { } finally { // Optimization to cleanup the cache if it is the last fragment boolean cleanImmediately = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); + LOG.debug("Releasing proxy user for session: " + session); try { proxyUGICache.release(session, cleanImmediately); } catch (Throwable t) { LOG.error("Error releasing UGICache for session: " + session, t); } - } } else { // no user impersonation is configured diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index 2b20eb9db6..84a681fcb3 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -245,6 +245,7 @@ public void releaseAnExpiredUGIResetsTheTimer() throws Exception { UserGroupInformation ugi2 = cache.getUserGroupInformation(session); assertEquals(ugi2, ugi1); verify(provider, never()).destroy(any(UserGroupInformation.class)); + // TODO: can we assert that the timer was reset } @Test From 14c9cdd8b07085ab00bd5093c862dfb96e25835d Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Fri, 13 Jul 2018 15:45:28 -0700 Subject: [PATCH 21/28] Fixed memory leak where the internal cache was not being cleaned up. Previously, when a UGI was fetched from the cache, we intended to remove all expired UGIs for the same segment, but we were not actually removing any. We were repeatedly calling cache.remove(session), where session was the key for the UGI fetched from the cache. We added tests and query methods to the cache to make sure the memory leak was fixed. Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 77 +++++++++++++++---- .../pxf/service/UGICacheMultiThreadTest.java | 4 + .../apache/hawq/pxf/service/UGICacheTest.java | 76 ++++++++++++------ 3 files changed, 115 insertions(+), 42 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 22fc6ba41d..9492594fde 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -76,11 +76,11 @@ public UserGroupInformation getUserGroupInformation(SessionId session) throws IO DelayQueue delayQueue = getExpirationQueue(segmentId); synchronized (delayQueue) { // Use the opportunity to cleanup any expired entries - cleanup(delayQueue, session); + cleanup(delayQueue); Entry entry = cache.get(session); if (entry == null) { LOG.debug(session.toString() + " Creating proxy user = " + user); - entry = new Entry(ticker, ugiProvider.createProxyUGI(user)); + entry = new Entry(ticker, ugiProvider.createProxyUGI(user), session); delayQueue.offer(entry); cache.put(session, entry); } @@ -111,7 +111,7 @@ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { entry.decrementRefCount(); expirationQueue.remove(entry); if (cleanImmediatelyIfNoRefs && entry.isNotInUse()) { - closeUGI(session, entry); + closeUGI(entry); } else { // Reset expiration time and put it back in the queue // only when we don't close the UGI @@ -121,6 +121,35 @@ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { } } + /** + * @return the size of the cache + */ + int size() { + return cache.size(); + } + + /** + * This method is not thread-safe + * + * @return the sum of the sizes of the internal queues + */ + int allQueuesSize() { + int count = 0; + for (DelayQueue queue : expirationQueueMap.values()) { + count += queue.size(); + } + return count; + } + + /** + * @param session + * @return determine whether the session is in the internal cache + */ + boolean contains(SessionId session) { + DelayQueue expirationQueue = getExpirationQueue(session.getSegmentId()); + return cache.containsKey(session) && expirationQueue.contains(cache.get(session)); + } + /** * Get the queue of cache entries associated with a segment, creating it if it doesn't * yet exist. This lets us lazily populate the expirationQueueMap. @@ -148,25 +177,24 @@ private DelayQueue getExpirationQueue(Integer segmentId) { * every non-expired entry. * * @param expirationQueue - * @param session */ - private void cleanup(DelayQueue expirationQueue, SessionId session) { + private void cleanup(DelayQueue expirationQueue) { Entry expiredUGI; while ((expiredUGI = expirationQueue.poll()) != null) { if (expiredUGI.isNotInUse()) { - closeUGI(session, expiredUGI); + closeUGI(expiredUGI); } else { // The UGI object is still being used by another thread - String fsMsg = "FileSystem for proxy user = " + session.getUser(); - LOG.debug(session.toString() + " Skipping close of " + fsMsg); + String fsMsg = "FileSystem for proxy user = " + expiredUGI.getSession().getUser(); + LOG.debug(expiredUGI.getSession().toString() + " Skipping close of " + fsMsg); // Place it back in the queue if still in use and was not closed expiredUGI.resetTime(); expirationQueue.offer(expiredUGI); } if (LOG.isDebugEnabled()) { LOG.debug("Delay Queue Size for segment " + - session.getSegmentId() + " = " + expirationQueue.size()); + expiredUGI.getSession().getSegmentId() + " = " + expirationQueue.size()); } } } @@ -176,20 +204,24 @@ private void cleanup(DelayQueue expirationQueue, SessionId session) { * session.getSegmentId(). When the reference count is 0, the Entry is removed from the * cache where it will then be processed to be destroyed by the UGIProvider. * - * @param session * @param expiredUGI * @return true if the UGI entry was cleaned, false when the UGI entry was still in use * and cleaning up was skipped */ - private void closeUGI(SessionId session, Entry expiredUGI) { + private void closeUGI(Entry expiredUGI) { // There is no need to synchronize this method because it should be called // from within a synchronized block + SessionId session = expiredUGI.getSession(); String fsMsg = "FileSystem for proxy user = " + session.getUser(); try { // Expired UGI object can be cleaned since it is not used - // Remove it from cache, as cache now has an - // expired entry which is not in progress - cache.remove(session); + // Determine if it can be removed from cache also + Entry cachedUGI = cache.get(session); + if (expiredUGI == cachedUGI) { + // Remove it from cache, as cache now has an + // expired entry which is not in progress + cache.remove(session); + } // Optimization to call close only if it has not been // called for that UGI @@ -213,20 +245,23 @@ private void closeUGI(SessionId session, Entry expiredUGI) { private static class Entry implements Delayed { private volatile long startTime; - private UserGroupInformation proxyUGI; - private boolean cleaned = false; + private final SessionId session; + private final UserGroupInformation proxyUGI; private final AtomicInteger referenceCount = new AtomicInteger(); private final Ticker ticker; + private boolean cleaned = false; /** * Creates a new UGICache Entry. * * @param ticker * @param proxyUGI + * @param session */ - Entry(Ticker ticker, UserGroupInformation proxyUGI) { + Entry(Ticker ticker, UserGroupInformation proxyUGI, SessionId session) { this.ticker = ticker; this.proxyUGI = proxyUGI; + this.session = session; } /** @@ -236,6 +271,14 @@ public UserGroupInformation getUGI() { return proxyUGI; } + + /** + * @return the session associated to the {@link UserGroupInformation}. + */ + public SessionId getSession() { + return session; + } + /** * @return true if the setCleaned has been invoked, false otherwise. */ diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java index b3a4f7bcc0..ea76b17041 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheMultiThreadTest.java @@ -123,5 +123,9 @@ public void run() { assertEquals(threadCount, finishedCount.intValue()); assertEquals(0, provider.countUgisInUse()); + // after the test has completed, the internal cache + // should be 0 + assertEquals(0, cache.size()); + assertEquals(0, cache.allQueuesSize()); } } diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index 84a681fcb3..b18027328b 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -29,10 +29,11 @@ import java.io.IOException; import java.util.concurrent.atomic.AtomicLong; +import static junit.framework.TestCase.assertTrue; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; import static org.mockito.Matchers.any; import static org.mockito.Mockito.doThrow; @@ -90,6 +91,8 @@ public void getSameUGITwiceUsesCache() throws Exception { UserGroupInformation ugi2 = cache.getUserGroupInformation(session); assertEquals(ugi1, ugi2); verify(provider, times(1)).createProxyUGI("the-user"); + assertEquals(1, cache.size()); + assertEquals(1, cache.allQueuesSize()); } @Test @@ -99,8 +102,12 @@ public void getTwoUGIsWithDifferentSessionsForSameUser() throws Exception { UserGroupInformation proxyUGI2 = cache.getUserGroupInformation(otherSession); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(2)).createProxyUGI("the-user"); - // TODO: this seems weird. We're creating two UGIs with the same params, - // even though we have two different sessions. Why? + assertEquals(2, cache.size()); + assertEquals(2, cache.allQueuesSize()); + // getting a new UGI instance for each transaction ID is not strictly necessary, but allows + // us to expire UGIs for transactions that have finished. If we reused one UGI per user, + // it might never get to expire from the cache, and eventually Kerberos might invalidate + // the UGI on its end. } @Test @@ -111,6 +118,7 @@ public void getTwoUGIsWithDifferentUsers() throws Exception { assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(1)).createProxyUGI("the-user"); verify(provider, times(1)).createProxyUGI("different-user"); + assertEquals(2, cache.size()); } @Test @@ -138,11 +146,16 @@ public void anySegmentIdIsValid() throws Exception { @Test public void ensureCleanUpAfterExpiration() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - fakeTicker.advanceTime(10 * MINUTES); cache.release(session, false); - fakeTicker.advanceTime(16 * MINUTES); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); + + SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); + cache.getUserGroupInformation(session2); assertNotInCache(session, ugi1); + cache.release(session2, true); + assertEquals(0, cache.size()); + assertEquals(0, cache.allQueuesSize()); } @Test @@ -166,7 +179,7 @@ public void putsItemsBackInTheQueueWhenResettingExpirationDate() throws Exceptio SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); SessionId session3 = new SessionId(0, "txn-id", "the-user-3"); - cache.getUserGroupInformation(session); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); fakeTicker.advanceTime(10 * MINUTES); UserGroupInformation ugi2 = cache.getUserGroupInformation(session2); cache.release(session2, false); @@ -175,7 +188,8 @@ public void putsItemsBackInTheQueueWhenResettingExpirationDate() throws Exceptio fakeTicker.advanceTime(2 * MINUTES); cache.getUserGroupInformation(session3); - verify(provider, times(1)).destroy(ugi2); + assertStillInCache(session, ugi1); + assertNotInCache(session2, ugi2); } @Test @@ -196,6 +210,21 @@ public void releaseWithForceClean() throws Exception { assertNotInCache(session, ugi1); } + @Test + public void releaseWithForceCleanOnlyCleansUGIsForThatSegment() throws Exception { + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + + SessionId session2 = new SessionId(999, "txn-id", "user"); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session2); + + cache.release(session2, false); // ugi2 is now unreferenced + cache.release(session, true); + assertNotInCache(session, ugi1); + assertStillInCache(session2, ugi2); + assertEquals(1, cache.size()); + assertEquals(1, cache.allQueuesSize()); + } + @Test public void releaseResetsTheExpirationTime() throws Exception { UserGroupInformation reference1 = cache.getUserGroupInformation(session); @@ -220,6 +249,7 @@ public void releaseAndReacquireDoesNotFreeResources() throws Exception { assertEquals(ugi3, ugi2); verify(provider, times(1)).createProxyUGI("the-user"); verify(provider, never()).destroy(any(UserGroupInformation.class)); + assertStillInCache(session, ugi2); } @Test @@ -228,11 +258,11 @@ public void releaseAndAcquireAfterTimeoutFreesResources() throws Exception { cache.release(session, false); fakeTicker.advanceTime(20 * MINUTES); - verify(provider, never()).destroy(any(UserGroupInformation.class)); + assertStillInCache(session, ugi1); UserGroupInformation ugi2 = cache.getUserGroupInformation(session); verify(provider).destroy(ugi1); assertNotEquals(ugi2, ugi1); - verify(provider, never()).destroy(ugi2); + assertStillInCache(session, ugi2); // this does not clean up any UGIs because our proxyUGI is still in use. verify(provider, times(2)).createProxyUGI("the-user"); } @@ -242,16 +272,19 @@ public void releaseAnExpiredUGIResetsTheTimer() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); fakeTicker.advanceTime(20 * MINUTES); cache.release(session, false); - UserGroupInformation ugi2 = cache.getUserGroupInformation(session); - assertEquals(ugi2, ugi1); - verify(provider, never()).destroy(any(UserGroupInformation.class)); - // TODO: can we assert that the timer was reset + + assertStillInCache(session, ugi1); + + fakeTicker.advanceTime(14 * MINUTES); + SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); + cache.getUserGroupInformation(session2); // triggers cleanup + assertStillInCache(session, ugi1); } @Test public void releaseDoesNotFreeResourcesIfUGIIsUsedElsewhere() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - cache.getUserGroupInformation(session); + cache.getUserGroupInformation(session); // increments ref count to 2 cache.release(session, true); fakeTicker.advanceTime(60 * MINUTES); @@ -267,12 +300,9 @@ public void releasingAllReferencesFreesResources() throws Exception { assertEquals(ugi1, ugi2); cache.release(session, true); - verify(provider, never()).destroy(any(UserGroupInformation.class)); + assertStillInCache(session, ugi1); cache.release(session, true); - verify(provider, times(1)).destroy(any(UserGroupInformation.class)); - - // at this point, the initial UGI has been freed. Calling - // getUserGroupInformation again creates a new UGI. + // at this point, the initial UGI has been freed. assertNotInCache(session, ugi1); } @@ -296,16 +326,12 @@ public void releaseAnEntryNotInTheCache() { } private void assertStillInCache(SessionId session, UserGroupInformation ugi) throws Exception { - UserGroupInformation fromCache = cache.getUserGroupInformation(session); - assertSame(ugi, fromCache); - cache.release(session, false); + assertTrue(cache.contains(session)); verify(provider, never()).destroy(ugi); } private void assertNotInCache(SessionId session, UserGroupInformation ugi) throws Exception { - UserGroupInformation fromCache = cache.getUserGroupInformation(session); - assertNotSame(ugi, fromCache); - cache.release(session, false); + assertFalse(cache.contains(session)); verify(provider, times(1)).destroy(ugi); } } From e7b9008146dc67600d336864e7837cdd20436193 Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Fri, 13 Jul 2018 16:46:43 -0700 Subject: [PATCH 22/28] Wrap LOG.debug calls in an if (LOG.isDebugEnabled) statement. Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 9492594fde..e72889567b 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -79,7 +79,9 @@ public UserGroupInformation getUserGroupInformation(SessionId session) throws IO cleanup(delayQueue); Entry entry = cache.get(session); if (entry == null) { - LOG.debug(session.toString() + " Creating proxy user = " + user); + if (LOG.isDebugEnabled()) { + LOG.debug(session + " Creating proxy user = " + user); + } entry = new Entry(ticker, ugiProvider.createProxyUGI(user), session); delayQueue.offer(entry); cache.put(session, entry); @@ -129,7 +131,7 @@ int size() { } /** - * This method is not thread-safe + * This method is not thread-safe, and is intended to be called in tests. * * @return the sum of the sizes of the internal queues */ @@ -142,12 +144,17 @@ int allQueuesSize() { } /** + * This method is O(n) in the number of cache entries and should only be called in tests. + * * @param session * @return determine whether the session is in the internal cache */ boolean contains(SessionId session) { DelayQueue expirationQueue = getExpirationQueue(session.getSegmentId()); - return cache.containsKey(session) && expirationQueue.contains(cache.get(session)); + synchronized (expirationQueue) { + Entry entry = cache.get(session); + return entry != null && expirationQueue.contains(entry); + } } /** @@ -187,7 +194,9 @@ private void cleanup(DelayQueue expirationQueue) { } else { // The UGI object is still being used by another thread String fsMsg = "FileSystem for proxy user = " + expiredUGI.getSession().getUser(); - LOG.debug(expiredUGI.getSession().toString() + " Skipping close of " + fsMsg); + if (LOG.isDebugEnabled()) { + LOG.debug(expiredUGI.getSession().toString() + " Skipping close of " + fsMsg); + } // Place it back in the queue if still in use and was not closed expiredUGI.resetTime(); expirationQueue.offer(expiredUGI); From 63b570a919b84b31122dc57da1d120f44679994f Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Fri, 13 Jul 2018 17:41:38 -0700 Subject: [PATCH 23/28] Wrap LOG.debug calls in an if (LOG.isDebugEnabled) statement. --- .../pxf/service/servlet/SecurityServletFilter.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 6c326fc33b..47459ad352 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -95,13 +95,17 @@ public void doFilter(final ServletRequest request, final ServletResponse respons PrivilegedExceptionAction action = new PrivilegedExceptionAction() { @Override public Boolean run() throws IOException, ServletException { - LOG.debug("Performing request chain call for proxy user = " + gpdbUser); + if (LOG.isDebugEnabled()) { + LOG.debug("Performing request chain call for proxy user = " + gpdbUser); + } chain.doFilter(request, response); return true; } }; - LOG.debug("Creating proxy user for session: " + session); + if (LOG.isDebugEnabled()) { + LOG.debug("Creating proxy user for session: " + session); + } try { // create proxy user UGI from the UGI of the logged in user // and execute the servlet chain as that user @@ -116,7 +120,9 @@ public Boolean run() throws IOException, ServletException { } finally { // Optimization to cleanup the cache if it is the last fragment boolean cleanImmediately = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); - LOG.debug("Releasing proxy user for session: " + session); + if (LOG.isDebugEnabled()) { + LOG.debug("Releasing proxy user for session: " + session); + } try { proxyUGICache.release(session, cleanImmediately); } catch (Throwable t) { From 9f183d0e7f40e1eb9795d7fe8da93ad2f67969b1 Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Mon, 16 Jul 2018 09:31:43 -0700 Subject: [PATCH 24/28] Add comment justifying our decision to use expirationQueue.remove --- .../src/main/java/org/apache/hawq/pxf/service/UGICache.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index e72889567b..b7110bbaf9 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -111,6 +111,12 @@ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { synchronized (expirationQueue) { entry.decrementRefCount(); + // expirationQueue.remove is an expensive operation. + // We envision that the number of elements in these queues + // will be at most in the hundreds of elements, which is + // why we went ahead with removing elements from the queue. + // If the use case changes, we should reconsider making the + // call below. expirationQueue.remove(entry); if (cleanImmediatelyIfNoRefs && entry.isNotInUse()) { closeUGI(entry); From bc514ee59f8461088b7692857bb439473c05dcb1 Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Mon, 16 Jul 2018 11:40:09 -0700 Subject: [PATCH 25/28] Refactored the JavaDocs and clean up tests Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 33 ++--- .../apache/hawq/pxf/service/UGICacheTest.java | 122 ++++++++---------- 2 files changed, 72 insertions(+), 83 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index b7110bbaf9..7dd218fb67 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -51,8 +51,8 @@ public class UGICache { final static long UGI_CACHE_EXPIRY = 15 * 60 * 1000L; // 15 Minutes /** - * Create a UGICache with the given {@link UGIProvider}. Intended for use by tests which need - * to substitute a mock UGIProvider. + * Create a UGICache with the given {@link Ticker} and {@link UGIProvider}. Intended for use by + * tests which need to mock UGI creation/destruction and the current time. */ UGICache(UGIProvider provider, Ticker ticker) { this.ticker = ticker; @@ -68,7 +68,12 @@ public UGICache() { } /** - * Create new proxy UGI if not found in cache and increment reference count + * If a UGI for the given session exists in the cache, returns it. Otherwise, creates a new + * proxy UGI. In either case this method increments the reference count of the UGI. This method + * also destroys expired, unreferenced UGIs for the same segmentId as the given session. + * + * @param session The user from the session is impersonated by the proxy UGI. + * @return the proxy UGI for the given session. */ public UserGroupInformation getUserGroupInformation(SessionId session) throws IOException { Integer segmentId = session.getSegmentId(); @@ -96,8 +101,8 @@ public UserGroupInformation getUserGroupInformation(SessionId session) throws IO * expire to UGI_CACHE_EXPIRY milliseconds in the future. * * @param session the session for which we want to release the UGI. - * @param cleanImmediatelyIfNoRefs if true, destroys the UGI for the given session (only if it is - * now unreferenced). + * @param cleanImmediatelyIfNoRefs if true, destroys the UGI for the given session (only if it + * is now unreferenced). */ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { @@ -164,8 +169,8 @@ boolean contains(SessionId session) { } /** - * Get the queue of cache entries associated with a segment, creating it if it doesn't - * yet exist. This lets us lazily populate the expirationQueueMap. + * Get the queue of cache entries associated with a segment, creating it if it doesn't yet + * exist. This lets us lazily populate the expirationQueueMap. * * @param segmentId * @return the {@link DelayQueue} associated to the segment. @@ -185,9 +190,8 @@ private DelayQueue getExpirationQueue(Integer segmentId) { } /** - * Iterate through all the entries in the queue and close expired - * {@link UserGroupInformation}, otherwise it resets the timer for - * every non-expired entry. + * Iterate through all the entries in the queue and close expired {@link UserGroupInformation}, + * otherwise it resets the timer for every non-expired entry. * * @param expirationQueue */ @@ -216,12 +220,10 @@ private void cleanup(DelayQueue expirationQueue) { /** * This method must be called from a synchronized block for the delayQueue for the given - * session.getSegmentId(). When the reference count is 0, the Entry is removed from the - * cache where it will then be processed to be destroyed by the UGIProvider. + * session.getSegmentId(). Removes the cachedUGI from the internal cache and then passes it to + * {@link UGIProvider} to destroy the UGI. * * @param expiredUGI - * @return true if the UGI entry was cleaned, false when the UGI entry was still in use - * and cleaning up was skipped */ private void closeUGI(Entry expiredUGI) { // There is no need to synchronize this method because it should be called @@ -369,7 +371,8 @@ private long getDelayMillis() { } /** - * @return the current Unix timestamp in milliseconds (equivalent to {@link System}.currentTimeMillis) + * @return the current Unix timestamp in milliseconds (equivalent to {@link + * System}.currentTimeMillis) */ private long currentTimeMillis() { return ticker.read() / 1000; diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index b18027328b..220b9faf84 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -91,19 +91,17 @@ public void getSameUGITwiceUsesCache() throws Exception { UserGroupInformation ugi2 = cache.getUserGroupInformation(session); assertEquals(ugi1, ugi2); verify(provider, times(1)).createProxyUGI("the-user"); - assertEquals(1, cache.size()); - assertEquals(1, cache.allQueuesSize()); + assertCacheSize(1); } @Test - public void getTwoUGIsWithDifferentSessionsForSameUser() throws Exception { + public void getTwoUGIsWithDifferentTransactionsForSameUser() throws Exception { SessionId otherSession = new SessionId(0, "txn-id-2", "the-user"); UserGroupInformation proxyUGI1 = cache.getUserGroupInformation(session); UserGroupInformation proxyUGI2 = cache.getUserGroupInformation(otherSession); assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(2)).createProxyUGI("the-user"); - assertEquals(2, cache.size()); - assertEquals(2, cache.allQueuesSize()); + assertCacheSize(2); // getting a new UGI instance for each transaction ID is not strictly necessary, but allows // us to expire UGIs for transactions that have finished. If we reused one UGI per user, // it might never get to expire from the cache, and eventually Kerberos might invalidate @@ -118,21 +116,9 @@ public void getTwoUGIsWithDifferentUsers() throws Exception { assertNotEquals(proxyUGI1, proxyUGI2); verify(provider, times(1)).createProxyUGI("the-user"); verify(provider, times(1)).createProxyUGI("different-user"); - assertEquals(2, cache.size()); - } - - @Test - public void gettingTwoUGIsWithDifferentUsersCachesBoth() throws Exception { - SessionId otherSession = new SessionId(0, "txn-id", "different-user"); - UserGroupInformation ugi1RefA = cache.getUserGroupInformation(session); - UserGroupInformation ugi1RefB = cache.getUserGroupInformation(session); - UserGroupInformation ugi2RefA = cache.getUserGroupInformation(otherSession); - UserGroupInformation ugi2RefB = cache.getUserGroupInformation(otherSession); - assertSame(ugi1RefA, ugi1RefB); - assertSame(ugi2RefA, ugi2RefB); - assertNotEquals(ugi1RefA, ugi2RefA); - verify(provider, times(1)).createProxyUGI("the-user"); - verify(provider, times(1)).createProxyUGI("different-user"); + assertCacheSize(2); + assertStillInCache(session, proxyUGI1); + assertStillInCache(otherSession, proxyUGI2); } @Test @@ -141,37 +127,37 @@ public void anySegmentIdIsValid() throws Exception { session = new SessionId(crazySegId, "txn-id", "the-user"); UserGroupInformation proxyUGI1 = cache.getUserGroupInformation(session); assertNotNull(proxyUGI1); + assertStillInCache(session, proxyUGI1); } @Test public void ensureCleanUpAfterExpiration() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - fakeTicker.advanceTime(10 * MINUTES); cache.release(session, false); fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); - cache.getUserGroupInformation(session2); - assertNotInCache(session, ugi1); + cache.getUserGroupInformation(session2); // this triggers cleanup of ugi1 + assertNoLongerInCache(session, ugi1); cache.release(session2, true); - assertEquals(0, cache.size()); - assertEquals(0, cache.allQueuesSize()); + assertCacheSize(0); } @Test - public void anUnusedUGIIsFreedAfterDelayWhenAnotherUGIForTheSameSegmentIsAccessed() throws Exception { + public void ensureExpiredUGIIsNotCleanedUpIfItIsStillReferenced() throws Exception { SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); - UserGroupInformation notInUse = cache.getUserGroupInformation(session); + UserGroupInformation stillInUse = cache.getUserGroupInformation(session); fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); - // at this point, notInUse is expired but still in use - cache.getUserGroupInformation(session2); + // at this point, stillInUse is expired but still in use + cache.getUserGroupInformation(session2); // trigger cleanup + assertStillInCache(session, stillInUse); cache.release(session, false); fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); cache.getUserGroupInformation(session2); - verify(provider, times(1)).destroy(notInUse); + verify(provider, times(1)).destroy(stillInUse); } @Test @@ -180,49 +166,46 @@ public void putsItemsBackInTheQueueWhenResettingExpirationDate() throws Exceptio SessionId session3 = new SessionId(0, "txn-id", "the-user-3"); UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - fakeTicker.advanceTime(10 * MINUTES); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY - 1000); UserGroupInformation ugi2 = cache.getUserGroupInformation(session2); cache.release(session2, false); - fakeTicker.advanceTime(14 * MINUTES); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY - 1000); cache.release(session, false); fakeTicker.advanceTime(2 * MINUTES); cache.getUserGroupInformation(session3); assertStillInCache(session, ugi1); - assertNotInCache(session2, ugi2); + assertNoLongerInCache(session2, ugi2); } @Test - public void releaseWithoutForceClean() throws Exception { + public void releaseWithoutImmediateCleanup() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); cache.release(session, false); - // UGI wasn't cleaned up, so we can still get it assertStillInCache(session, ugi1); - verify(provider, times(1)).createProxyUGI("the-user"); } @Test - public void releaseWithForceClean() throws Exception { + public void releaseWithImmediateCleanup() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); cache.release(session, true); - assertNotInCache(session, ugi1); + assertNoLongerInCache(session, ugi1); } @Test - public void releaseWithForceCleanOnlyCleansUGIsForThatSegment() throws Exception { + public void releaseWithImmediateCleanupOnlyCleansUGIsForThatSegment() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - SessionId session2 = new SessionId(999, "txn-id", "user"); - UserGroupInformation ugi2 = cache.getUserGroupInformation(session2); + SessionId differentSeg = new SessionId(999, "txn-id", "user"); + UserGroupInformation ugi2 = cache.getUserGroupInformation(differentSeg); - cache.release(session2, false); // ugi2 is now unreferenced + cache.release(differentSeg, false); // ugi2 is now unreferenced cache.release(session, true); - assertNotInCache(session, ugi1); - assertStillInCache(session2, ugi2); - assertEquals(1, cache.size()); - assertEquals(1, cache.allQueuesSize()); + assertNoLongerInCache(session, ugi1); + assertStillInCache(differentSeg, ugi2); + assertCacheSize(1); } @Test @@ -231,19 +214,33 @@ public void releaseResetsTheExpirationTime() throws Exception { cache.getUserGroupInformation(session); cache.release(session, true); - fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); cache.release(session, false); - fakeTicker.advanceTime(10 * MINUTES); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY - 1000); assertStillInCache(session, reference1); } + @Test + public void releaseAnExpiredUGIResetsTheTimer() throws Exception { + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); + cache.release(session, false); + + assertStillInCache(session, ugi1); + + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY - 1000); + SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); + cache.getUserGroupInformation(session2); // triggers cleanup + assertStillInCache(session, ugi1); + } + @Test public void releaseAndReacquireDoesNotFreeResources() throws Exception { cache.getUserGroupInformation(session); cache.release(session, false); UserGroupInformation ugi2 = cache.getUserGroupInformation(session); - fakeTicker.advanceTime(50 * MINUTES); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); UserGroupInformation ugi3 = cache.getUserGroupInformation(session); // this does not clean up any UGIs because our ugi is still in use. assertEquals(ugi3, ugi2); @@ -257,28 +254,12 @@ public void releaseAndAcquireAfterTimeoutFreesResources() throws Exception { UserGroupInformation ugi1 = cache.getUserGroupInformation(session); cache.release(session, false); - fakeTicker.advanceTime(20 * MINUTES); + fakeTicker.advanceTime(UGICache.UGI_CACHE_EXPIRY + 1000); assertStillInCache(session, ugi1); UserGroupInformation ugi2 = cache.getUserGroupInformation(session); verify(provider).destroy(ugi1); assertNotEquals(ugi2, ugi1); assertStillInCache(session, ugi2); - // this does not clean up any UGIs because our proxyUGI is still in use. - verify(provider, times(2)).createProxyUGI("the-user"); - } - - @Test - public void releaseAnExpiredUGIResetsTheTimer() throws Exception { - UserGroupInformation ugi1 = cache.getUserGroupInformation(session); - fakeTicker.advanceTime(20 * MINUTES); - cache.release(session, false); - - assertStillInCache(session, ugi1); - - fakeTicker.advanceTime(14 * MINUTES); - SessionId session2 = new SessionId(0, "txn-id", "the-user-2"); - cache.getUserGroupInformation(session2); // triggers cleanup - assertStillInCache(session, ugi1); } @Test @@ -303,7 +284,7 @@ public void releasingAllReferencesFreesResources() throws Exception { assertStillInCache(session, ugi1); cache.release(session, true); // at this point, the initial UGI has been freed. - assertNotInCache(session, ugi1); + assertNoLongerInCache(session, ugi1); } @Test(expected = IOException.class) @@ -330,8 +311,13 @@ private void assertStillInCache(SessionId session, UserGroupInformation ugi) thr verify(provider, never()).destroy(ugi); } - private void assertNotInCache(SessionId session, UserGroupInformation ugi) throws Exception { + private void assertNoLongerInCache(SessionId session, UserGroupInformation ugi) throws Exception { assertFalse(cache.contains(session)); verify(provider, times(1)).destroy(ugi); } + + private void assertCacheSize(int expectedSize) { + assertEquals(expectedSize, cache.size()); + assertEquals(expectedSize, cache.allQueuesSize()); + } } From 3a1b1a9129f6cb504f6ac6f5834d59a1b7fd3c4b Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Mon, 16 Jul 2018 15:03:40 -0700 Subject: [PATCH 26/28] UGICache: Remove comment and redundant checks Co-authored-by: Francisco Guerrero Co-authored-by: Ben Christel --- .../org/apache/hawq/pxf/service/UGICache.java | 52 ++++--------------- .../servlet/SecurityServletFilter.java | 29 +++++------ .../apache/hawq/pxf/service/UGICacheTest.java | 9 +++- 3 files changed, 30 insertions(+), 60 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java index 7dd218fb67..febb3ed9e4 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/UGICache.java @@ -116,12 +116,6 @@ public void release(SessionId session, boolean cleanImmediatelyIfNoRefs) { synchronized (expirationQueue) { entry.decrementRefCount(); - // expirationQueue.remove is an expensive operation. - // We envision that the number of elements in these queues - // will be at most in the hundreds of elements, which is - // why we went ahead with removing elements from the queue. - // If the use case changes, we should reconsider making the - // call below. expirationQueue.remove(entry); if (cleanImmediatelyIfNoRefs && entry.isNotInUse()) { closeUGI(entry); @@ -226,30 +220,19 @@ private void cleanup(DelayQueue expirationQueue) { * @param expiredUGI */ private void closeUGI(Entry expiredUGI) { - // There is no need to synchronize this method because it should be called - // from within a synchronized block SessionId session = expiredUGI.getSession(); String fsMsg = "FileSystem for proxy user = " + session.getUser(); - try { - // Expired UGI object can be cleaned since it is not used - // Determine if it can be removed from cache also - Entry cachedUGI = cache.get(session); - if (expiredUGI == cachedUGI) { - // Remove it from cache, as cache now has an - // expired entry which is not in progress - cache.remove(session); - } - // Optimization to call close only if it has not been - // called for that UGI - if (!expiredUGI.isCleaned()) { - if (LOG.isDebugEnabled()) { - LOG.debug(session.toString() + " Closing " + fsMsg + - " (Cache Size = " + cache.size() + ")"); - } - ugiProvider.destroy(expiredUGI.getUGI()); - expiredUGI.setCleaned(); - } + if (LOG.isDebugEnabled()) { + LOG.debug(session.toString() + " Closing " + fsMsg + + " (Cache Size = " + cache.size() + ")"); + } + + try { + // Remove it from cache, as cache now has an + // expired entry which is not in progress + cache.remove(session); + ugiProvider.destroy(expiredUGI.getUGI()); } catch (Throwable t) { LOG.warn(session.toString() + " Error closing " + fsMsg, t); @@ -266,7 +249,6 @@ private static class Entry implements Delayed { private final UserGroupInformation proxyUGI; private final AtomicInteger referenceCount = new AtomicInteger(); private final Ticker ticker; - private boolean cleaned = false; /** * Creates a new UGICache Entry. @@ -296,20 +278,6 @@ public SessionId getSession() { return session; } - /** - * @return true if the setCleaned has been invoked, false otherwise. - */ - boolean isCleaned() { - return cleaned; - } - - /** - * mark that the {@link UserGroupInformation} in this Entry has been destroyed. - */ - void setCleaned() { - cleaned = true; - } - /** * @return true if the UGI is being referenced by a session, false otherwise */ diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 47459ad352..e710471e26 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -50,7 +50,7 @@ public class SecurityServletFilter implements Filter { private static final String FRAGMENT_COUNT_HEADER = "X-GP-FRAGMENT-COUNT"; private static final String MISSING_HEADER_ERROR = "Header %s is missing in the request"; private static final String EMPTY_HEADER_ERROR = "Header %s is empty in the request"; - private static UGICache proxyUGICache; + private UGICache proxyUGICache; /** * Initializes the filter. @@ -78,8 +78,8 @@ public void doFilter(final ServletRequest request, final ServletResponse respons if (SecureLogin.isUserImpersonationEnabled()) { // retrieve user header and make sure header is present and is not empty - final String gpdbUser = getHeaderValue(request, USER_HEADER); - String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER); + final String gpdbUser = getHeaderValue(request, USER_HEADER, true); + String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER, true); Integer segmentId = getHeaderValueInt(request, SEGMENT_ID_HEADER, true); Integer fragmentCount = getHeaderValueInt(request, FRAGMENT_COUNT_HEADER, false); Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); @@ -104,10 +104,10 @@ public Boolean run() throws IOException, ServletException { }; if (LOG.isDebugEnabled()) { - LOG.debug("Creating proxy user for session: " + session); + LOG.debug("Retrieving proxy user for session: " + session); } try { - // create proxy user UGI from the UGI of the logged in user + // Retrieve proxy user UGI from the UGI of the logged in user // and execute the servlet chain as that user proxyUGICache .getUserGroupInformation(session) @@ -135,17 +135,19 @@ public Boolean run() throws IOException, ServletException { } } + /** + * Destroys the filter. + */ + @Override + public void destroy() { + } + private Integer getHeaderValueInt(ServletRequest request, String headerKey, boolean required) throws IllegalArgumentException { String value = getHeaderValue(request, headerKey, required); return value != null ? Integer.valueOf(value) : null; } - private String getHeaderValue(ServletRequest request, String headerKey) - throws IllegalArgumentException { - return getHeaderValue(request, headerKey, true); - } - private String getHeaderValue(ServletRequest request, String headerKey, boolean required) throws IllegalArgumentException { String value = ((HttpServletRequest) request).getHeader(headerKey); @@ -157,11 +159,4 @@ private String getHeaderValue(ServletRequest request, String headerKey, boolean return value; } - /** - * Destroys the filter. - */ - @Override - public void destroy() { - } - } diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java index 220b9faf84..44c3f9a54a 100644 --- a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/UGICacheTest.java @@ -34,7 +34,6 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertSame; import static org.mockito.Matchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; @@ -94,6 +93,14 @@ public void getSameUGITwiceUsesCache() throws Exception { assertCacheSize(1); } + @Test + public void getUGIWithEquivalentSessionsReturnsTheSameInstance() throws Exception { + SessionId session2 = new SessionId(0, "txn-id", "the-user"); + UserGroupInformation ugi1 = cache.getUserGroupInformation(session); + UserGroupInformation ugi2 = cache.getUserGroupInformation(session2); + assertEquals(ugi1, ugi2); + } + @Test public void getTwoUGIsWithDifferentTransactionsForSameUser() throws Exception { SessionId otherSession = new SessionId(0, "txn-id-2", "the-user"); From 0d64656a1b6b7224ec2444a560f6c28673e6f196 Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Tue, 17 Jul 2018 11:54:44 -0700 Subject: [PATCH 27/28] Clean UGI cache when X-GP-LAST-FRAGMENT header is 'true' - Added unit tests for SecurityServletFilter Co-authored-by: Ben Christel Co-authored-by: Francisco Guerrero --- .../servlet/SecurityServletFilter.java | 22 ++-- .../servlet/SecurityServletFilterTest.java | 114 ++++++++++++++++++ 2 files changed, 125 insertions(+), 11 deletions(-) create mode 100644 pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilterTest.java diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index e710471e26..6f078b3a42 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -31,6 +31,7 @@ import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import org.apache.commons.lang.StringUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hawq.pxf.service.SessionId; @@ -46,11 +47,10 @@ public class SecurityServletFilter implements Filter { private static final String USER_HEADER = "X-GP-USER"; private static final String SEGMENT_ID_HEADER = "X-GP-SEGMENT-ID"; private static final String TRANSACTION_ID_HEADER = "X-GP-XID"; - private static final String FRAGMENT_INDEX_HEADER = "X-GP-FRAGMENT-INDEX"; - private static final String FRAGMENT_COUNT_HEADER = "X-GP-FRAGMENT-COUNT"; + private static final String LAST_FRAGMENT_HEADER = "X-GP-LAST-FRAGMENT"; private static final String MISSING_HEADER_ERROR = "Header %s is missing in the request"; private static final String EMPTY_HEADER_ERROR = "Header %s is empty in the request"; - private UGICache proxyUGICache; + UGICache proxyUGICache; /** * Initializes the filter. @@ -81,13 +81,9 @@ public void doFilter(final ServletRequest request, final ServletResponse respons final String gpdbUser = getHeaderValue(request, USER_HEADER, true); String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER, true); Integer segmentId = getHeaderValueInt(request, SEGMENT_ID_HEADER, true); - Integer fragmentCount = getHeaderValueInt(request, FRAGMENT_COUNT_HEADER, false); - Integer fragmentIndex = getHeaderValueInt(request, FRAGMENT_INDEX_HEADER, false); + boolean lastCallForSegment = getHeaderValueBoolean(request, LAST_FRAGMENT_HEADER, false); SessionId session = new SessionId(segmentId, transactionId, gpdbUser); - if (LOG.isDebugEnabled() && fragmentCount != null) { - LOG.debug(session.toString() + " Fragment = " + fragmentIndex + " of " + fragmentCount); - } // TODO refresh Kerberos token when security is enabled @@ -119,12 +115,12 @@ public Boolean run() throws IOException, ServletException { throw new ServletException(ie); } finally { // Optimization to cleanup the cache if it is the last fragment - boolean cleanImmediately = (fragmentIndex != null && fragmentIndex.equals(fragmentCount)); if (LOG.isDebugEnabled()) { - LOG.debug("Releasing proxy user for session: " + session); + LOG.debug("Releasing proxy user for session: " + session + + (lastCallForSegment ? " Last fragment call." : "")); } try { - proxyUGICache.release(session, cleanImmediately); + proxyUGICache.release(session, lastCallForSegment); } catch (Throwable t) { LOG.error("Error releasing UGICache for session: " + session, t); } @@ -159,4 +155,8 @@ private String getHeaderValue(ServletRequest request, String headerKey, boolean return value; } + private boolean getHeaderValueBoolean(ServletRequest request, String headerKey, boolean required) { + return StringUtils.equals("true", getHeaderValue(request, headerKey, required)); + } + } diff --git a/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilterTest.java b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilterTest.java new file mode 100644 index 0000000000..c4b2c27010 --- /dev/null +++ b/pxf/pxf-service/src/test/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilterTest.java @@ -0,0 +1,114 @@ +package org.apache.hawq.pxf.service.servlet; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hawq.pxf.service.SessionId; +import org.apache.hawq.pxf.service.UGICache; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import javax.servlet.FilterChain; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; + +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class SecurityServletFilterTest { + + private static final String PROPERTY_KEY_USER_IMPERSONATION = "pxf.service.user.impersonation.enabled"; + + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private HttpServletRequest servletRequest; + + @Before + public void setup() { + System.setProperty(PROPERTY_KEY_USER_IMPERSONATION, "true"); + servletRequest = mock(HttpServletRequest.class); + } + + @Test + public void throwsWhenRequiredHeaderIsEmpty() throws Exception { + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Header X-GP-USER is empty in the request"); + + when(servletRequest.getHeader("X-GP-USER")).thenReturn(" "); + + SecurityServletFilter securityServletFilter = new SecurityServletFilter(); + securityServletFilter.doFilter(servletRequest, mock(ServletResponse.class), mock(FilterChain.class)); + + } + + @Test + public void throwsWhenRequiredHeaderIsMissing() throws Exception { + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Header X-GP-USER is missing in the request"); + + SecurityServletFilter securityServletFilter = new SecurityServletFilter(); + securityServletFilter.doFilter(servletRequest, mock(ServletResponse.class), mock(FilterChain.class)); + } + + @Test + public void doesNotCleanTheUGICacheOnNonLastCalls() throws Exception { + + when(servletRequest.getHeader("X-GP-USER")).thenReturn("gpadmin"); + when(servletRequest.getHeader("X-GP-XID")).thenReturn("0"); + when(servletRequest.getHeader("X-GP-SEGMENT-ID")).thenReturn("1"); + + SecurityServletFilter securityServletFilter = new SecurityServletFilter(); + securityServletFilter.proxyUGICache = mock(UGICache.class); + + when(securityServletFilter.proxyUGICache.getUserGroupInformation(any(SessionId.class))).thenReturn(mock(UserGroupInformation.class)); + + securityServletFilter.doFilter(servletRequest, mock(ServletResponse.class), mock(FilterChain.class)); + + verify(securityServletFilter.proxyUGICache).release(any(SessionId.class), eq(false)); + } + + @Test + public void tellsTheUGICacheToCleanItselfOnTheLastCallForASegment() throws Exception { + + System.setProperty(PROPERTY_KEY_USER_IMPERSONATION, "true"); + HttpServletRequest servletRequest = mock(HttpServletRequest.class); + + when(servletRequest.getHeader("X-GP-USER")).thenReturn("gpadmin"); + when(servletRequest.getHeader("X-GP-XID")).thenReturn("0"); + when(servletRequest.getHeader("X-GP-SEGMENT-ID")).thenReturn("1"); + when(servletRequest.getHeader("X-GP-LAST-FRAGMENT")).thenReturn("true"); + + SecurityServletFilter securityServletFilter = new SecurityServletFilter(); + securityServletFilter.proxyUGICache = mock(UGICache.class); + + when(securityServletFilter.proxyUGICache.getUserGroupInformation(any(SessionId.class))).thenReturn(mock(UserGroupInformation.class)); + + securityServletFilter.doFilter(servletRequest, mock(ServletResponse.class), mock(FilterChain.class)); + + verify(securityServletFilter.proxyUGICache).release(any(SessionId.class), eq(true)); + } +} From b44a42b5d3480152616508ac4c78e8084e627fb6 Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Tue, 17 Jul 2018 17:19:34 -0700 Subject: [PATCH 28/28] Add final to variables for consistency --- .../hawq/pxf/service/servlet/SecurityServletFilter.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java index 6f078b3a42..0c48cca4f7 100644 --- a/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java +++ b/pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/servlet/SecurityServletFilter.java @@ -79,9 +79,9 @@ public void doFilter(final ServletRequest request, final ServletResponse respons // retrieve user header and make sure header is present and is not empty final String gpdbUser = getHeaderValue(request, USER_HEADER, true); - String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER, true); - Integer segmentId = getHeaderValueInt(request, SEGMENT_ID_HEADER, true); - boolean lastCallForSegment = getHeaderValueBoolean(request, LAST_FRAGMENT_HEADER, false); + final String transactionId = getHeaderValue(request, TRANSACTION_ID_HEADER, true); + final Integer segmentId = getHeaderValueInt(request, SEGMENT_ID_HEADER, true); + final boolean lastCallForSegment = getHeaderValueBoolean(request, LAST_FRAGMENT_HEADER, false); SessionId session = new SessionId(segmentId, transactionId, gpdbUser);