Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11561 )
Change subject: IMPALA-7626: Throttle catalog partial RPC requests ...................................................................... Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/11561/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11561/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2119 PS7, Line 2119: ( > nit: weird formatting Oops IDE issues. http://gerrit.cloudera.org:8080/#/c/11561/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2131 PS7, Line 2131: // this method. > is there a chance that we get interrupted right on this line and then miss Like we discussed offline, there is a very very minor chance of the thread getting interrupted at this point and if that happens, the permit gets blocked forever. Good part though is that the subsequent requests should throw the time-out exception and is easy to diagnose and if someone looks at the queue length after seeing those exceptions. There is another variant of this method which uses an Atomic variable to see if it has been set. But that is equally suscpetible to this problem if the thread gets interrupted right after tryAcquire() is successful but getAndSet is not run. I'll add a comment here about it. public TGetPartialCatalogObjectResponse getPartialCatalogObject (TGetPartialCatalogObjectRequest req) throws CatalogException { AtomicBoolean isPermitAcquired = new AtomicBoolean(false); try { isPermitAcquired.getAndSet(partialObjectFetchAccess_.tryAcquire(1, PARTIAL_FETCH_RPC_QUEUE_TIMEOUT_S, TimeUnit.SECONDS)); if (!isPermitAcquired.get()) { // Timed out trying to acquire the semaphore permit. throw new CatalogException("Timed out while fetching partial object metadata. " + "Please check the metric 'catalog.partial-fetch-rpc.queue-len' for the " + "current queue length and consider increasing " + "'catalog_partial_fetch_rpc_queue_timeout_s' and/or " + "'catalog_max_parallel_partial_fetch_rpc'"); } return doGetPartialCatalogObject(req); } catch (InterruptedException e) { throw new CatalogException("Error running getPartialCatalogObject(): ", e); } finally { if (isPermitAcquired.get()) partialObjectFetchAccess_.release(); } } -- To view, visit http://gerrit.cloudera.org:8080/11561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11f77a16cfa38ada42d8b7c859850198ea7dd142 Gerrit-Change-Number: 11561 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 05 Oct 2018 01:52:42 +0000 Gerrit-HasComments: Yes
