[impala] branch branch-4.0.1 updated (80a1dc3 -> edca349)

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stigahuang pushed a change to branch branch-4.0.1
in repository https://gitbox.apache.org/repos/asf/impala.git.


    from 80a1dc3  IMPALA-10714: Defer advancing read page until the buffer is attached
     new ce433ba  IMPALA-10936: Fix NPE in collecting policy tables on FailedLoadLocalTable
     new edca349  IMPALA-10942: Fix memory leak in admission controller

The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 be/src/scheduling/admission-control-service.cc     |  2 +-
 be/src/scheduling/admission-controller.cc          |  6 +++---
 be/src/scheduling/admission-controller.h           |  2 +-
 .../scheduling/local-admission-control-client.cc   |  2 +-
 .../apache/impala/analysis/StmtMetadataLoader.java |  9 ++++++--
 .../apache/impala/catalog/local/LocalTable.java    | 18 ++++++++--------
 .../impala/analysis/StmtMetadataLoaderTest.java    | 25 +++++++++++++++++++++-
 7 files changed, 46 insertions(+), 18 deletions(-)

[impala] 02/02: IMPALA-10942: Fix memory leak in admission controller

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stigahuang pushed a commit to branch branch-4.0.1
in repository https://gitbox.apache.org/repos/asf/impala.git

commit edca349527657a9847659d7b0400bf62aa6938ce
Author: Bikramjeet Vig <bi...@gmail.com>
AuthorDate: Thu Sep 30 19:03:15 2021 -0700

    IMPALA-10942: Fix memory leak in admission controller
    
    This patch fixes a memory leak in the admission controller where
    objects tracking state required for queuing were being retained
    despite the query being admitted or rejected immediately.
    
    Change-Id: I1df0de0e658f6dcb5a044d108d0943aaa3dea0a1
    Reviewed-on: http://gerrit.cloudera.org:8080/17893
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/scheduling/admission-control-service.cc      | 2 +-
 be/src/scheduling/admission-controller.cc           | 6 +++---
 be/src/scheduling/admission-controller.h            | 2 +-
 be/src/scheduling/local-admission-control-client.cc | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/be/src/scheduling/admission-control-service.cc b/be/src/scheduling/admission-control-service.cc
index f00f335..748107f 100644
--- a/be/src/scheduling/admission-control-service.cc
+++ b/be/src/scheduling/admission-control-service.cc
@@ -323,7 +323,7 @@ void AdmissionControlService::AdmitFromThreadPool(UniqueIdPB query_id) {
         admission_state->blacklisted_executor_addresses};
     admission_state->admit_status =
         AdmissiondEnv::GetInstance()->admission_controller()->SubmitForAdmission(request,
-            &admission_state->admit_outcome, &admission_state->schedule, &queued,
+            &admission_state->admit_outcome, &admission_state->schedule, queued,
             &admission_state->request_pool);
     admission_state->submitted = true;
     if (!queued) {
diff --git a/be/src/scheduling/admission-controller.cc b/be/src/scheduling/admission-controller.cc
index 1d1f544..9aebfad 100644
--- a/be/src/scheduling/admission-controller.cc
+++ b/be/src/scheduling/admission-controller.cc
@@ -1133,9 +1133,9 @@ void AdmissionController::PoolStats::UpdateConfigMetrics(const TPoolConfig& pool
 
 Status AdmissionController::SubmitForAdmission(const AdmissionRequest& request,
     Promise<AdmissionOutcome, PromiseMode::MULTIPLE_PRODUCER>* admit_outcome,
-    unique_ptr<QuerySchedulePB>* schedule_result, bool* queued,
+    unique_ptr<QuerySchedulePB>* schedule_result, bool& queued,
     std::string* request_pool) {
-  *queued = false;
+  queued = false;
   DebugActionNoFail(request.query_options, "AC_BEFORE_ADMISSION");
   DCHECK(schedule_result->get() == nullptr);
 
@@ -1249,7 +1249,7 @@ Status AdmissionController::SubmitForAdmission(const AdmissionRequest& request,
       PROFILE_INFO_KEY_INITIAL_QUEUE_REASON, queue_node->initial_queue_reason);
 
   queue_node->wait_start_ms = MonotonicMillis();
-  *queued = true;
+  queued = true;
   return Status::OK();
 }
 
diff --git a/be/src/scheduling/admission-controller.h b/be/src/scheduling/admission-controller.h
index fb8bca2..635ceac 100644
--- a/be/src/scheduling/admission-controller.h
+++ b/be/src/scheduling/admission-controller.h
@@ -369,7 +369,7 @@ class AdmissionController {
   /// cancelled to ensure that the pool statistics are updated.
   Status SubmitForAdmission(const AdmissionRequest& request,
       Promise<AdmissionOutcome, PromiseMode::MULTIPLE_PRODUCER>* admit_outcome,
-      std::unique_ptr<QuerySchedulePB>* schedule_result, bool* queued,
+      std::unique_ptr<QuerySchedulePB>* schedule_result, bool& queued,
       std::string* request_pool = nullptr);
 
   /// After SubmitForAdmission(), if the query was queued this must be called. If
diff --git a/be/src/scheduling/local-admission-control-client.cc b/be/src/scheduling/local-admission-control-client.cc
index faf47a5..e85f34b 100644
--- a/be/src/scheduling/local-admission-control-client.cc
+++ b/be/src/scheduling/local-admission-control-client.cc
@@ -38,7 +38,7 @@ Status LocalAdmissionControlClient::SubmitForAdmission(
   query_events->MarkEvent(QUERY_EVENT_SUBMIT_FOR_ADMISSION);
   bool queued;
   Status status = ExecEnv::GetInstance()->admission_controller()->SubmitForAdmission(
-      request, &admit_outcome_, schedule_result, &queued);
+      request, &admit_outcome_, schedule_result, queued);
   if (queued) {
     query_events->MarkEvent(QUERY_EVENT_QUEUED);
     DCHECK(status.ok());

[impala] 01/02: IMPALA-10936: Fix NPE in collecting policy tables on FailedLoadLocalTable

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stigahuang pushed a commit to branch branch-4.0.1
in repository https://gitbox.apache.org/repos/asf/impala.git

commit ce433ba42e8816217212de08ab5b6552b3cb9280
Author: stiga-huang <hu...@gmail.com>
AuthorDate: Thu Sep 30 10:39:26 2021 +0800

    IMPALA-10936: Fix NPE in collecting policy tables on FailedLoadLocalTable
    
    Ranger column-masking/row-filtering policies can have complex
    expressions that reference other tables (e.g. by using subqueries). So
    when loading metadata of a table, we also need to load all tables
    referenced by its related policies (i.e. "policy tables"). This requires
    getting all its columns so we can get the column-masking policies.
    However, for a failed loaded table in LocalCatalog mode, the table is
    represented by a FailedLoadLocalTable which has a null ColumnMap. This
    causes a NullPointerException when collecting policy tables for it.
    
    This patch skips collecting policy tables on failed loaded tables. Also
    add some null checks in LocalTable in case its ColumnMap is null. So
    FailedLoadLocalTable can have the same robustness as IncompleteTable
    which is the failed table representation in the legacy catalog mode.
    
    Tests:
     - Manually verified the NPE disappear in logs
     - Added FE unit test
    
    Change-Id: I7a6cd567685920211f05f2fdaac96bc98da41ac6
    Reviewed-on: http://gerrit.cloudera.org:8080/17888
    Reviewed-by: Joe McDonnell <jo...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 .../apache/impala/analysis/StmtMetadataLoader.java |  9 ++++++--
 .../apache/impala/catalog/local/LocalTable.java    | 18 ++++++++--------
 .../impala/analysis/StmtMetadataLoaderTest.java    | 25 +++++++++++++++++++++-
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java b/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
index fa8ecc4..5ed2f3b 100644
--- a/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
+++ b/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
@@ -18,6 +18,7 @@
 package org.apache.impala.analysis;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.concurrent.TimeUnit;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -44,6 +45,7 @@ import org.apache.impala.util.TUniqueIdUtil;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 
 /**
@@ -315,7 +317,8 @@ public class StmtMetadataLoader {
         viewTbls.addAll(collectTableCandidates(((FeView) tbl).getQueryStmt()));
       }
       // Adds tables/views introduced by column-masking/row-filtering policies.
-      if (fe_.getAuthzFactory().getAuthorizationConfig().isEnabled()
+      if (!(tbl instanceof FeIncompleteTable)
+          && fe_.getAuthzFactory().getAuthorizationConfig().isEnabled()
           && fe_.getAuthzFactory().supportsTableMasking() && user_ != null) {
         try {
           viewTbls.addAll(collectPolicyTables(tbl));
@@ -346,8 +349,10 @@ public class StmtMetadataLoader {
     return tableNames;
   }
 
-  private Set<TableName> collectPolicyTables(FeTable tbl)
+  @VisibleForTesting
+  Set<TableName> collectPolicyTables(FeTable tbl)
       throws InternalException, AnalysisException {
+    if (tbl instanceof FeIncompleteTable) return Collections.emptySet();
     Set<TableName> tableNames = new HashSet<>();
     String dbName = tbl.getDb().getName();
     String tblName = tbl.getName();
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
index d89b960..44928a8 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java
@@ -18,6 +18,7 @@
 package org.apache.impala.catalog.local;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 import javax.annotation.Nullable;
@@ -210,8 +211,7 @@ abstract class LocalTable implements FeTable {
 
   @Override
   public List<Column> getColumns() {
-    // TODO(todd) why does this return ArrayList instead of List?
-    return new ArrayList<>(cols_.colsByPos_);
+    return cols_ == null ? Collections.emptyList() : cols_.colsByPos_;
   }
 
   @Override
@@ -228,37 +228,37 @@ abstract class LocalTable implements FeTable {
 
   @Override
   public List<String> getColumnNames() {
-    return cols_.getColumnNames();
+    return cols_ == null ? Collections.emptyList() : cols_.getColumnNames();
   }
 
   @Override
   public List<Column> getClusteringColumns() {
-    return cols_.getClusteringColumns();
+    return cols_ == null ? Collections.emptyList() : cols_.getClusteringColumns();
   }
 
   @Override
   public List<Column> getNonClusteringColumns() {
-    return cols_.getNonClusteringColumns();
+    return cols_ == null ? Collections.emptyList() : cols_.getNonClusteringColumns();
   }
 
   @Override
   public int getNumClusteringCols() {
-    return cols_.getNumClusteringCols();
+    return cols_ == null ? 0 : cols_.getNumClusteringCols();
   }
 
   @Override
   public boolean isClusteringColumn(Column c) {
-    return cols_.isClusteringColumn(c);
+    return cols_ != null && cols_.isClusteringColumn(c);
   }
 
   @Override
   public Column getColumn(String name) {
-    return cols_.getByName(name);
+    return cols_ == null ? null : cols_.getByName(name);
   }
 
   @Override
   public ArrayType getType() {
-    return cols_.getType();
+    return cols_ == null ? null : cols_.getType();
   }
 
   @Override
diff --git a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
index 730a02e..8a50b8e 100644
--- a/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
@@ -21,17 +21,24 @@ import java.util.Arrays;
 
 import org.apache.impala.analysis.StmtMetadataLoader.StmtTableCache;
 import org.apache.impala.authorization.NoopAuthorizationFactory;
+import org.apache.impala.authorization.User;
 import org.apache.impala.catalog.Catalog;
 import org.apache.impala.catalog.FeTable;
+import org.apache.impala.catalog.TableLoadingException;
+import org.apache.impala.catalog.local.CatalogdMetaProvider;
+import org.apache.impala.catalog.local.FailedLoadLocalTable;
+import org.apache.impala.catalog.local.LocalCatalog;
+import org.apache.impala.catalog.local.LocalDb;
 import org.apache.impala.common.ImpalaException;
 import org.apache.impala.common.InternalException;
 import org.apache.impala.compat.MetastoreShim;
+import org.apache.impala.service.BackendConfig;
+import org.apache.impala.service.FeSupport;
 import org.apache.impala.service.Frontend;
 import org.apache.impala.testutil.ImpaladTestCatalog;
 import org.apache.impala.util.EventSequence;
 import org.junit.Assert;
 import org.junit.Assume;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public class StmtMetadataLoaderTest {
@@ -232,4 +239,20 @@ public class StmtMetadataLoaderTest {
     Assume.assumeTrue(MetastoreShim.getMajorVersion() >= 3);
     testLoadAcidTables("select * from functional.insert_only_transactional_table");
   }
+
+  @Test
+  public void testCollectPolicyTablesOnFailedTables() throws ImpalaException {
+    FeSupport.loadLibrary();
+    CatalogdMetaProvider provider = new CatalogdMetaProvider(
+        BackendConfig.INSTANCE.getBackendCfg());
+    LocalCatalog catalog = new LocalCatalog(provider, /*defaultKuduMasterHosts=*/null);
+    Frontend fe = new Frontend(new NoopAuthorizationFactory(), catalog);
+    EventSequence timeline = new EventSequence("Test Timeline");
+    User user = new User("user");
+    StmtMetadataLoader mdLoader =
+        new StmtMetadataLoader(fe, Catalog.DEFAULT_DB, timeline, user);
+    LocalDb db = new LocalDb(catalog, "default");
+    mdLoader.collectPolicyTables(new FailedLoadLocalTable(
+        db, "tbl", new TableLoadingException("error", /*cause=*/new Exception())));
+  }
 }