Vihang Karajgaonkar has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/16548 )

Change subject: IMPALA-10219: Expose DEBUG_ACTION query option in catalog
......................................................................


Patch Set 5:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/16548/5/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/16548/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2284
PS5, Line 2284:   public TCatalogObject reloadTable(Table tbl, 
TResetMetadataRequest request, String reason)
> line too long (92 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1152
PS5, Line 1152:               storageMetadataLoadTime_ += 
updateUnpartitionedTableFileMd(client, debugAction);
> line too long (94 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@945
PS5, Line 945:             reloadFileMetadata, reloadTableSchema, false, 
partitionsToUpdate, null, reason);
> line too long (92 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3702
PS5, Line 3702:     List<List<String>> partitionsNotInHms = 
hdfsTable.getPathsWithoutPartitions(debugAction);
> line too long (93 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/util/DebugUtils.java
File fe/src/main/java/org/apache/impala/util/DebugUtils.java:

http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/util/DebugUtils.java@29
PS5, Line 29: backend
> I think that it would be useful to add a reference in the BE and FE impleme
good point. I updated the code documentation to a pointer from be to fe and 
vice-versa.


http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/util/DebugUtils.java@72
PS5, Line 72:       if (components.get(0).equalsIgnoreCase(label)) {
> maybe use continue in the !equal case to reduce nesting?
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/main/java/org/apache/impala/util/DebugUtils.java@78
PS5, Line 78:         Preconditions.checkState(actionParams.size() > 1,
            :             "Illegal debug action format found in " + 
debugActions + " for label"
            :                 + label);
> This seems consistent with BE, but I don't understand why some format error
yes, make sense to validate the debug action when it is set instead of when it 
is evaluated. Created IMPALA-10268 for this.


http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/test/java/org/apache/impala/util/DebugUtilsTest.java
File fe/src/test/java/org/apache/impala/util/DebugUtilsTest.java:

http://gerrit.cloudera.org:8080/#/c/16548/5/fe/src/test/java/org/apache/impala/util/DebugUtilsTest.java@32
PS5, Line 32:     
DebugUtils.executeDebugAction("TEST_SLEEP_ACTION:SLEEP@100|SOME_OTHER_ACTION:SLEEP@10",
> line too long (91 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/tests/metadata/test_catalogd_debug_actions.py
File tests/metadata/test_catalogd_debug_actions.py:

http://gerrit.cloudera.org:8080/#/c/16548/5/tests/metadata/test_catalogd_debug_actions.py@21
PS5, Line 21: class TestDebugActions(ImpalaTestSuite):
> flake8: E302 expected 2 blank lines, found 1
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/tests/metadata/test_catalogd_debug_actions.py@48
PS5, Line 48:
> flake8: E203 whitespace before ':'
Done


http://gerrit.cloudera.org:8080/#/c/16548/5/tests/metadata/test_catalogd_debug_actions.py@50
PS5, Line 50:
> flake8: W391 blank line at end of file
Done



--
To view, visit http://gerrit.cloudera.org:8080/16548
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7196b1ce76415a5faf3fa8575a26d22b2bf50b1
Gerrit-Change-Number: 16548
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Oct 2020 21:40:32 +0000
Gerrit-HasComments: Yes

Reply via email to