[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21029 )

Change subject: IMPALA-12543: Detect self-events before finishing DDL
..


Patch Set 9:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15402/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 06:58:29 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21029 )

Change subject: IMPALA-12543: Detect self-events before finishing DDL
..


Patch Set 8:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15401/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 06:48:14 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21029 )

Change subject: IMPALA-12543: Detect self-events before finishing DDL
..


Patch Set 9:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@909
PS7, Line 909:   LOG.info("Received {} events. First event id: {}.", 
response.getEvents().size(),
 :   (response.getEvents().size() > 0 ? 
response.getEvents().get(0).getEventId() :
 :  "none"));
 :   if (filter
> nit: It'd be more readable to use the message template:
Done


http://gerrit.cloudera.org:8080/#/c/21029/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/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1683
PS5, Line 1683: if (msTbl.getPartitionKeysSize() ==
> +1. Could you add a comment for this?
Done


http://gerrit.cloudera.org:8080/#/c/21029/7/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/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1066
PS7, Line 1066:  * Register the new version number into table's in-flight 
events.
> Could you add a comment for when should we use this, e.g. before invoking t
Done


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@
PS7, Line :  */
  : public void validateInProgressModificationComplete() {
> nit: please add error messages for these
Done


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1366
PS7, Line 1366:   }
> nit: let's add an error message for Preconditions.
Done


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4493
PS7, Line 4493:   }
> Might be an existing issue, but should we still do this when 'addedHmsParti
Probably not, but I prefer matching the old-behavior like in old-code, L1147.
We can investigate the impact of not doing this (and another seemingly 
unnecessary code in L5307) in separate patch and see which test will break.


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5819
PS7, Line 5819: KuduCatalogOpExecutor.validateKuduTblExists(msTbl);
> Just realized this is invoked in alterTable() instead of alterView().. In a
This probably need to be handled by the switch anyway for SET_VIEW_PROPERTIES 
enum value.


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6135
PS7, Line 6135: }
> Should we do this before L6106 where the events will be generated, and hand
This is one spot where I'm no sure about. What if some alterPartitions went 
through and the rest is not?

Fortunately, I think partition-level events handling still acquire table locks.

>From IMPALA-12461 commit message:

"Postponing solving this for partition level events as that would
be more complex, as both the partition list and the partitions'
in-flight event lists would need to be protected from parallel
operations that add/remove partitions."

So cancelInflightEvent is not required here, just like other alter partition 
operation (ie., alterTableDropPartition and alterTableAddPartitions).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 06:36:59 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL

2024-03-04 Thread Riza Suminto (Code Review)
Hello Quanlong Huang, Sai Hemanth Gantasala, Csaba Ringhofer, Impala Public 
Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21029

to look at the new patch set (#9).

Change subject: IMPALA-12543: Detect self-events before finishing DDL
..

IMPALA-12543: Detect self-events before finishing DDL

test_iceberg_self_events has been flaky for not having
tbls_refreshed_before equal to tbls_refreshed_after in-between query
executions. This flakiness happens in ARM, TEST_JDK_VERSION 11, and
TEST_JDK_VERSION 17 environments.

Further investigation reveals concurrency bug due to db/table level
locks is not taken during db/table self-events check (IMPALA-12461
part1). The order of ALTER TABLE operation is as follow:

1. alter table starts in CatalogOpExecutor
2. table level lock is taken
3. HMS RPC starts (CatalogOpExecutor.applyAlterTable())
4. HMS generates the event
5. HMS RPC returns
6. table is reloaded
7. catalog version is added to inflight event list
8. table level lock is releases

Meanwhile the event processor thread fetches the new event after 4 and
before 7. Because of IMPALA-12461 (part 1), it can also finish
self-events checking before reaching 7. Before IMPALA-12461, self-events
would have needed to wait for 8. Note that this issue is only relevant
for table level events, as self-events checking for partition level
events still takes table lock.

This patch fix the issue by adding newCatalogVersion to the table's
inflight event list before updating HMS. If HMS update does not
complete (ie., an exception is thrown), the new newCatalogVersion that
was added is then removed.

This patch also fix few smaller issues, including:
- Avoid incrementing EVENTS_SKIPPED_METRIC if numFilteredEvents == 0 in
  MetastoreEventFactory.getFilteredEvents().
- Increment EVENTS_SKIPPED_METRIC in
  MetastoreTableEvent.reloadTableFromCatalog() if table is already in
  the middle of reloading (revealed through flaky
  test_skipping_older_events).
- Rephrase misleading log message in
  MetastoreEventProcessor.getNextMetastoreEvents().

Testing:
- Pass exhaustive tests.

Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 273 insertions(+), 141 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/21029/9
--
To view, visit http://gerrit.cloudera.org:8080/21029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 


[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL

2024-03-04 Thread Riza Suminto (Code Review)
Hello Quanlong Huang, Sai Hemanth Gantasala, Csaba Ringhofer, Impala Public 
Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21029

to look at the new patch set (#8).

Change subject: IMPALA-12543: Detect self-events before finishing DDL
..

IMPALA-12543: Detect self-events before finishing DDL

test_iceberg_self_events has been flaky for not having
tbls_refreshed_before equal to tbls_refreshed_after in-between query
executions. This flakiness happens in ARM, TEST_JDK_VERSION 11, and
TEST_JDK_VERSION 17 environments.

Further investigation reveals concurrency bug due to db/table level
locks is not taken during db/table self-events check (IMPALA-12461
part1). The order of ALTER TABLE operation is as follow:

1. alter table starts in CatalogOpExecutor
2. table level lock is taken
3. HMS RPC starts (CatalogOpExecutor.applyAlterTable())
4. HMS generates the event
5. HMS RPC returns
6. table is reloaded
7. catalog version is added to inflight event list
8. table level lock is releases

Meanwhile the event processor thread fetches the new event after 4 and
before 7. Because of IMPALA-12461 (part 1), it can also finish
self-events checking before reaching 7. Before IMPALA-12461, self-events
would have needed to wait for 8. Note that this issue is only relevant
for table level events, as self-events checking for partition level
events still takes table lock.

This patch fix the issue by adding newCatalogVersion to the table's
inflight event list before updating HMS. If HMS update does not
complete (ie., an exception is thrown), the new newCatalogVersion that
was added is then removed.

This patch also fix few smaller issues, including:
- Avoid incrementing EVENTS_SKIPPED_METRIC if numFilteredEvents == 0 in
  MetastoreEventFactory.getFilteredEvents().
- Increment EVENTS_SKIPPED_METRIC in
  MetastoreTableEvent.reloadTableFromCatalog() if table is already in
  the middle of reloading (revealed through flaky
  test_skipping_older_events).
- Rephrase misleading log message in
  MetastoreEventProcessor.getNextMetastoreEvents().

Testing:
- Pass exhaustive tests.

Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 270 insertions(+), 141 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/21029/8
--
To view, visit http://gerrit.cloudera.org:8080/21029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 


[Impala-ASF-CR] IMPALA-12802: Support ALTER TABLE for JDBC tables

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21088 )

Change subject: IMPALA-12802: Support ALTER TABLE for JDBC tables
..


Patch Set 3:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15400/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ebb5de2c686d2015db78641f78299dd5f33621e
Gerrit-Change-Number: 21088
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Reviewer: gaurav singh 
Gerrit-Comment-Date: Tue, 05 Mar 2024 06:14:17 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12802: Support ALTER TABLE for JDBC tables

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21088 )

Change subject: IMPALA-12802: Support ALTER TABLE for JDBC tables
..


Patch Set 2:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15399/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ebb5de2c686d2015db78641f78299dd5f33621e
Gerrit-Change-Number: 21088
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou 
Gerrit-Reviewer: Abhishek Rawat 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Wenzhe Zhou 
Gerrit-Reviewer: gaurav singh 
Gerrit-Comment-Date: Tue, 05 Mar 2024 06:03:49 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12802: Support ALTER TABLE for JDBC tables

2024-03-04 Thread Wenzhe Zhou (Code Review)
Wenzhe Zhou has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/21088 )

Change subject: IMPALA-12802: Support ALTER TABLE for JDBC tables
..

IMPALA-12802: Support ALTER TABLE for JDBC tables

IMPALA-12793 changes the syntax for creating JDBC table. The
configurations of connection credentials - url, username, password,
jdbc driver, etc, are set as table properties.

This patch allows user to change these table properties, or edit
columns via ALTER TABLE statement.

Testing:
 - Added frontend analysis unit-tests.
 - Added end-to-end unit-test.
 - Passed Core tests

Change-Id: I5ebb5de2c686d2015db78641f78299dd5f33621e
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableRecoverPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetCachedStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetColumnStats.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetFileFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetLocationStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetOwnerStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableSetPartitionSpecStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewUnSetTblProperties.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/FeDataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M testdata/workloads/functional-query/queries/QueryTest/jdbc-data-source.test
33 files changed, 453 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/21088/3
--
To view, visit http://gerrit.cloudera.org:8080/21088
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5ebb5de2c686d2015db78641f78299dd5f33621e
Gerrit-Change-Number: 21088
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Wenzhe Zhou 


[Impala-ASF-CR] IMPALA-12802: Support ALTER TABLE for JDBC tables

2024-03-04 Thread Wenzhe Zhou (Code Review)
Wenzhe Zhou has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21088


Change subject: IMPALA-12802: Support ALTER TABLE for JDBC tables
..

IMPALA-12802: Support ALTER TABLE for JDBC tables

IMPALA-12793 changes the syntax for creating JDBC table. The
configurations of connection credentials - url, username, password,
jdbc driver, etc, are set as table properties.

This patch allows user to change these table properties, or edit
columns via ALTER TABLE statement.

Testing:
 - Added frontend analysis unit-tests.
 - Added end-to-end unit-test.
 - Passed Core tests

Change-Id: I5ebb5de2c686d2015db78641f78299dd5f33621e
---
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropColStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewRenameStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableOrViewSetOwnerStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableRecoverPartitionsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetCachedStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetColumnStats.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetFileFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetLocationStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetOwnerStmt.java
M 
fe/src/main/java/org/apache/impala/analysis/AlterTableSetPartitionSpecStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableSortByStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewSetOwnerStmt.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/AlterViewUnSetTblProperties.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/FeDataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M testdata/workloads/functional-query/queries/QueryTest/jdbc-data-source.test
33 files changed, 453 insertions(+), 33 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/88/21088/2
--
To view, visit http://gerrit.cloudera.org:8080/21088
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5ebb5de2c686d2015db78641f78299dd5f33621e
Gerrit-Change-Number: 21088
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou 


[Impala-ASF-CR] IMPALA-12802: Support ALTER TABLE for JDBC tables

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21088 )

Change subject: IMPALA-12802: Support ALTER TABLE for JDBC tables
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21088/2/fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java
File 
fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java:

http://gerrit.cloudera.org:8080/#/c/21088/2/fe/src/main/java/org/apache/impala/analysis/AlterTableUnSetTblProperties.java@157
PS2, Line 157: throw new AnalysisException(String.format("Unsetting the 
'%s' table property is " +
line too long (91 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ebb5de2c686d2015db78641f78299dd5f33621e
Gerrit-Change-Number: 21088
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 05 Mar 2024 05:39:40 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12771: Impala catalogd events-skipped may mark the wrong number

2024-03-04 Thread Anonymous Coward (Code Review)
cclive1...@gmail.com has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21045 )

Change subject: IMPALA-12771: Impala catalogd events-skipped may mark the wrong 
number
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21045/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/21045/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1055
PS1, Line 1055: } else if (numPartsRefreshed == -1) {
> I think using "else" without the condition is more robust in case the retur
Hi, the reason that I change the catalogOpExecutor_.reloadPartitionsIfExist 
result from 0 to -1 for cases like : table does not exist or table is 
IncompleteTable. This means the table has no need to reload the partition, -1 
is just a marker for me , But if the result is 0, that may means the 
function(hdfsTable.reloadPartitionsFromNames() to do real partition may has 
been executed but the result is 0 which means the event is not skipped but been 
executed.


http://gerrit.cloudera.org:8080/#/c/21045/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1056
PS1, Line 1056: inc
> Please add a debug log when increasing this counter.
ok


http://gerrit.cloudera.org:8080/#/c/21045/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2341
PS1, Line 2341: debugLog("Incremented skipped metric to " + metrics_
> We can improve this to add the reason - "since no partitions were added"
ok



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7aeb04e999b82187eb138c0b643ead259da22f1a
Gerrit-Change-Number: 21045
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Comment-Date: Tue, 05 Mar 2024 03:07:43 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 10: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Tue, 05 Mar 2024 02:51:29 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..

IMPALA-12864: Deflake test_query_log_size_in_bytes.

test_query_log_size_in_bytes is flaky in exhaustive builds. The expected
QueryStateRecord is off by around 100KB per query, indicating that the
test query might be too complex and lead to variability in final query
profile that is being archived.

This patch simplifies the test query and relaxes the assertion. This
patch also adds QUERY_LOG_EST_TOTAL_BYTES metric (key name
"impala-server.query-log-est-total-bytes") that is validated as well in
test_query_log_size_in_bytes. query-state-record-test.cc is modified a
bit to resolve segmentation fault issue when building
unified-be-test-validated-executable target run_clang_tidy.sh.

Testing:
- Pass test_query_log_size_in_bytes in exhaustive build.

Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Reviewed-on: http://gerrit.cloudera.org:8080/21100
Reviewed-by: Impala Public Jenkins 
Tested-by: Impala Public Jenkins 
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record-test.cc
M be/src/service/query-state-record.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M bin/run_clang_tidy.sh
M common/thrift/metrics.json
M tests/custom_cluster/test_web_pages.py
10 files changed, 55 insertions(+), 29 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 11
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 11:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15398/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 11
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 02:39:38 +
Gerrit-HasComments: No


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 10:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15397/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 10
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 02:37:00 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax

2024-03-04 Thread Andrew Sherman (Code Review)
Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21083 )

Change subject: IMPALA-12774: [DOCS] Document ALTER TABLE SORT BY syntax
..


Patch Set 1:

(2 comments)

This is a welcome addition, I had some ideas about structuring it differently.

http://gerrit.cloudera.org:8080/#/c/21083/1/docs/topics/impala_alter_table.xml
File docs/topics/impala_alter_table.xml:

http://gerrit.cloudera.org:8080/#/c/21083/1/docs/topics/impala_alter_table.xml@515
PS1, Line 515:   To change the column or order the records are sorted 
by:
I found this feature a bit confusing. I tried to come up with a simple summary, 
the best I could come up with quickly was "To specify a sort order for new 
records that are added to the table"


http://gerrit.cloudera.org:8080/#/c/21083/1/docs/topics/impala_alter_table.xml@521
PS1, Line 521:   Specifying the sort order is optional. The default sort 
order is LEXICAL.
This seems useful.
I wonder if this should be merged into the doc in
https://impala.apache.org/docs/build/html/topics/impala_create_table.html
as that has a lot of good detail.
Then maybe we can somehow have a summary here and link to create table for the 
details.
What do you think?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb348d8395a6140f0be200d73e2f22fded9a5116
Gerrit-Change-Number: 21083
Gerrit-PatchSet: 1
Gerrit-Owner: Noemi Pap-Takacs 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Tue, 05 Mar 2024 02:28:48 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Sai Hemanth Gantasala (Code Review)
Sai Hemanth Gantasala has uploaded a new patch set (#11) to the change 
originally created by k.venureddy2...@gmail.com. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..

[WIP]IMPALA-12832: Implicit invalidate metadata on event failures

At present, failure in event processing needs manual invalidate
metadata. This patch implicitly invalidates the table upon failures
in processing of table events with new 'process_event_failure' flag.
And a new 'auto_global_invalidate_metadata' flag is added to global
invalidate metadata automatically when event processor goes to
non-active state.

Note: Also introduced a config
'inject_process_event_failure_event_types' for automated tests to
simulate event processor failures. This config is used to specify what
event types can be intentionally failed. This config should only be
used for testing purpose.

Testing:
- Added end-to-end tests to mimic failures in event processor and verified
that event processor is active
- Added unit test to verify the 'auto_global_invalidate_metadata' config
- Passed FE tests

Co-Authored-by: Sai Hemanth Gantasala 

Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
---
M be/src/catalog/catalog-server.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M 
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/common/impala_test_suite.py
A tests/metadata/__init__.py
M tests/metadata/test_event_processing.py
A tests/metadata/test_event_processing_base.py
A tests/metadata/test_event_processing_error.py
M tests/util/event_processor_utils.py
16 files changed, 934 insertions(+), 305 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/21065/11
--
To view, visit http://gerrit.cloudera.org:8080/21065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 11
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 11:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py
File tests/metadata/test_event_processing_error.py:

http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py@47
PS11, Line 47:
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py@47
PS11, Line 47:   catalogd_args="--catalog_topic_mode=minimal "
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py@48
PS11, Line 48:
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py@48
PS11, Line 48: "--process_event_failure=true "
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py@281
PS11, Line 281: #
flake8: E265 block comment should start with '# '


http://gerrit.cloudera.org:8080/#/c/21065/11/tests/metadata/test_event_processing_error.py@286
PS11, Line 286: +
flake8: E226 missing whitespace around arithmetic operator



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 11
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 02:15:28 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 10:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py
File tests/metadata/test_event_processing_error.py:

http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@47
PS10, Line 47: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@48
PS10, Line 48: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@63
PS10, Line 63: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@64
PS10, Line 64: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@80
PS10, Line 80: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@81
PS10, Line 81: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@99
PS10, Line 99: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@100
PS10, Line 100: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@119
PS10, Line 119: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@120
PS10, Line 120: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@136
PS10, Line 136: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@137
PS10, Line 137: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@157
PS10, Line 157: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@158
PS10, Line 158: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@176
PS10, Line 176: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@177
PS10, Line 177: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@210
PS10, Line 210: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@211
PS10, Line 211: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@212
PS10, Line 212: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@213
PS10, Line 213: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@231
PS10, Line 231: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@232
PS10, Line 232: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@246
PS10, Line 246: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@247
PS10, Line 247: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@261
PS10, Line 261: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@262
PS10, Line 262: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/21065/10/tests/metadata/test_event_processing_error.py@263
PS10, Line 263

[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Sai Hemanth Gantasala (Code Review)
Sai Hemanth Gantasala has uploaded a new patch set (#10) to the change 
originally created by k.venureddy2...@gmail.com. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..

[WIP]IMPALA-12832: Implicit invalidate metadata on event failures

At present, failure in event processing needs manual invalidate
metadata. This patch implicitly invalidates the table upon failures
in processing of table events with new 'process_event_failure' flag.
And a new 'auto_global_invalidate_metadata' flag is added to global
invalidate metadata automatically when event processor goes to
non-active state.

Note: Also introduced a config
'inject_process_event_failure_event_types' for automated tests to
simulate event processor failures. This config is used to specify what
event types can be intentionally failed. This config should only be
used for testing purpose.

Testing:
- Added end-to-end tests to mimic failures in event processor and verified
that event processor is active
- Added unit test to verify the 'auto_global_invalidate_metadata' config
- Passed FE tests

Co-Authored-by: Sai Hemanth Gantasala 

Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
---
M be/src/catalog/catalog-server.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M 
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M tests/common/impala_test_suite.py
A tests/metadata/__init__.py
M tests/metadata/test_event_processing.py
A tests/metadata/test_event_processing_base.py
A tests/metadata/test_event_processing_error.py
M tests/util/event_processor_utils.py
16 files changed, 934 insertions(+), 305 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/21065/10
--
To view, visit http://gerrit.cloudera.org:8080/21065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 10
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 


[Impala-ASF-CR] IMPALA-12543: Detect self-events before finishing DDL

2024-03-04 Thread Quanlong Huang (Code Review)
Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21029 )

Change subject: IMPALA-12543: Detect self-events before finishing DDL
..


Patch Set 7:

(8 comments)

Thanks for fixing this! It's a great work that requires detailed reviews. Left 
some comments first. I'll look deeper into the patch.

http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@909
PS7, Line 909:   LOG.info("Received " + response.getEvents().size() + " 
events. First event id : "
 :   + (response.getEvents().size() > 0 ? 
response.getEvents().get(0).getEventId() :
 :"none")
 :   + ".");
nit: It'd be more readable to use the message template:

LOG.info("Received {} events. First event id: {}.", response.getEvents().size(),
(response.getEvents().size() > 0 ? response.getEvents().get(0).getEventId() 
:
"none"));


http://gerrit.cloudera.org:8080/#/c/21029/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/21029/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1683
PS5, Line 1683:
> I don't think that in-flight events are needed here. As for a view reloadin
+1. Could you add a comment for this?


http://gerrit.cloudera.org:8080/#/c/21029/7/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/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1066
PS7, Line 1066:  * Register the new version number into table's in-flight 
events.
Could you add a comment for when should we use this, e.g. before invoking the 
HMS API for non-insert events?


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@
PS7, Line :   
Preconditions.checkState(!table_.hasInProgressModification());
  :   Preconditions.checkState(!inflightEventRegistered_);
nit: please add error messages for these


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1366
PS7, Line 1366: Preconditions.checkState(reloadMetadata);
nit: let's add an error message for Preconditions.


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4493
PS7, Line 4493: modification.registerInflightEvent();
Might be an existing issue, but should we still do this when 
'addedHmsPartitions' is empty, i.e. no partitions are added?


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5819
PS7, Line 5819:   private void alterViewSetTblProperties(Table tbl,
Just realized this is invoked in alterTable() instead of alterView().. In 
alterView(), we don't deal with the inflight versions (at L1754). Maybe we 
should make them consistent. But not a big deal since reload on views should be 
fast, unless it's a materialized view (IIRC, we can't alter MV yet).


http://gerrit.cloudera.org:8080/#/c/21029/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6135
PS7, Line 6135: modification.registerInflightEvent();
Should we do this before L6106 where the events will be generated, and handle 
the failure at L6128?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8365c934349ad21a4d9327fc11594d2fc3445f79
Gerrit-Change-Number: 21029
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 02:05:38 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 31:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc
File be/src/service/workload-management.cc:

http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@589
PS31, Line 589:   if(LIKELY(!ctx.record->per_host_state.empty())) {
nit: space after 'if'



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 31
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Tue, 05 Mar 2024 01:12:12 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 31:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc
File be/src/service/workload-management.cc:

http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@575
PS31, Line 575:   auto min_elem = 
max_element(ctx.record->per_host_state.cbegin(),
Should be max_elem.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 31
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Tue, 05 Mar 2024 01:11:20 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 9: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10351/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Tue, 05 Mar 2024 00:08:34 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 31:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/internal-server.cc
File be/src/service/internal-server.cc:

http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/internal-server.cc@117
PS13, Line 117: !UUIDEmpty(internal_query_id)) {
> Use the new function in uid-util.h
Done


http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc
File be/src/service/workload-management.cc:

http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@646
PS12, Line 646:   fields_.push_back(MakeTuple("exec_summary", _exec_summary));
  :   fields_.push_back(MakeTuple("plan", _plan));
  :   fields_.push_back(MakeTuple("num_rows_fetched", 
_num_rows_fetched, "BIGINT"));
  :   fields_.push_back(MakeTuple(
  :   "row_materialization_bytes_per_sec", 
_row_materialization_rate, "BIGINT"));
  :   fields_.push_back(MakeTuple(
  :   "row_materializ
> I'm going to rearrange these into a different order that puts the string ch
Done


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc
File be/src/service/workload-management.cc:

http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@171
PS31, Line 171: SHUTDOWN_REQUESTED
nit: SHUTTING_DOWN


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@769
PS31, Line 769: completed_queries_thread_state_ == RUNNING
Can ShutdownWorkloadManagement called before completed_queries_thread_state_ 
becomes RUNNING?


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@841
PS31, Line 841: completed_queries_thread_state_
What if the previous completed_queries_thread_state_ is SHUTDOWN_REQUESTED or 
SHUTDOWN here?


http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/network-util.h
File be/src/util/network-util.h:

http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/network-util.h@114
PS13, Line 114: turn lhs.hostname() == rhs.hostname() && lhs.port() == 
rhs.port()
> Should uds_address be the last tie breaker?
Done


http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py
File tests/beeswax/impala_beeswax.py:

http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py@185
PS31, Line 185: fetch_profile_after_close=False
fetch_profile_after_close=True seems specific to TestQueryLogTable tests only.

Can this variable turned into a Client field member and set during Client 
initialization?
Tests that require this behavior can then specify it during create_connection().


http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py@198
PS31, Line 198: runtime_profile = self.get_runtime_profile(handle)
Skip pulling profile if !fetch_profile_after_close


http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py@219
PS31, Line 219: result.runtime_profile = self.get_runtime_profile(handle)
Skip pulling profile if !fetch_profile_after_close


http://gerrit.cloudera.org:8080/#/c/20770/31/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:

http://gerrit.cloudera.org:8080/#/c/20770/31/tests/common/custom_cluster_test_suite.py@244
PS31, Line 244: method=None
this is unused?


http://gerrit.cloudera.org:8080/#/c/20770/9/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/20770/9/tests/custom_cluster/test_query_log.py@359
PS9, Line 359: se:
 :   assert exec_group is not None
 :   assert data[index] == exec_group.group(1), "executor
> I didn't know it was possible to get the profile from the query handle.  I
Done


http://gerrit.cloudera.org:8080/#/c/20770/12/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/20770/12/tests/custom_cluster/test_query_log.py@633
PS12, Line 633: 
catalogd_args="--enable_workload_mgmt",
  : 
impalad_graceful_shutdown=True)
  :   @pytest.mark.parametrize("client_protocol", PROTOCOL_ALL)
  :   def test_query_log_table_ddl(self, client_protocol):
  : """Asserts the values written to the query log table match 
the values from the
  :query
> Maybe it is due to stale metadata. Is it worth to run "invalidate metadata
Moved discussion to patch set 31.


http://gerrit.cloudera.org:8080/#/c/20770/31/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/20770/31/tests/custom_cluster/test_query_log.py

[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 23:37:20 +
Gerrit-HasComments: No


[native-toolchain-CR] IMPALA-12863: Fix toolchain container builds for CentOS 7, SLES 12 & 15

2024-03-04 Thread Joe McDonnell (Code Review)
Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21091 )

Change subject: IMPALA-12863: Fix toolchain container builds for CentOS 7, SLES 
12 & 15
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21091/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21091/3//COMMIT_MSG@18
PS3, Line 18: but the Kudu
: build requires the 'hostname' utility from it.
Can you add "hostname" to the list of required executables in 
docker/all/assert-dependencies-present.py?



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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e730abb6667bf00735ea62c35377591b68452ce
Gerrit-Change-Number: 21091
Gerrit-PatchSet: 3
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 04 Mar 2024 23:18:38 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 31:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20770/22/be/src/service/impala-http-handler.h
File be/src/service/impala-http-handler.h:

http://gerrit.cloudera.org:8080/#/c/20770/22/be/src/service/impala-http-handler.h@210
PS22, Line 210:   void QueryStateToJson(const QueryStateRecord& record,
> Shouldn't this be updated in the parent patch? Same with impala-http-handle
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 31
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 22:42:34 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 31:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/20770/22/be/src/service/impala-http-handler.h
File be/src/service/impala-http-handler.h:

http://gerrit.cloudera.org:8080/#/c/20770/22/be/src/service/impala-http-handler.h@210
PS22, Line 210:   void QueryStateToJson(const QueryStateRecord& record,
Shouldn't this be updated in the parent patch? Same with impala-http-handler.cc 
changes. Please do a test build with each patch to make sure it still compiles 
on its own, rather than requiring the full change stack.


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc
File be/src/service/workload-management.cc:

http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@218
PS31, Line 218:   for (auto iter = sql.cbegin(); iter != sql.cend(); iter++) {
Could be a range-based for loop taking 'const auto&' (always a good default).


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@393
PS31, Line 393: for (auto iter = ctx.record->per_host_state.cbegin();
Could be a range-based for loop taking 'const auto&'.


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@659
PS31, Line 659: fields_.push_back(MakeTuple(
Lines 659-677 should be indented.


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@687
PS31, Line 687:   for (auto iter = fields_.cbegin(); iter != fields_.cend(); 
iter++) {
Could be a range-based for loop (const auto&).


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@726
PS31, Line 726:   for (auto iter = fields_.cbegin(); iter != fields_.cend(); 
iter++) {
Range-based for loop.


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@895
PS31, Line 895:   for (auto iter = queries_to_insert.begin(); iter != 
queries_to_insert.end();
Could be a range-based for loop taking a ref (to update the count).


http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@953
PS31, Line 953:   for (auto iter = fields_.cbegin(); iter != fields_.cend(); 
iter++) {
Range-based for loop.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 31
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 22:41:56 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 9:

(1 comment)

Thanks all for the review!

http://gerrit.cloudera.org:8080/#/c/21100/9/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/21100/9/be/src/service/impala-server.cc@1167
PS9, Line 1167: 
ImpaladMetrics::QUERY_LOG_EST_TOTAL_BYTES->Increment(record_size);
> It would make sense to me for this to be tied to the server rather than a g
Right, it looks weird to maintain a state in ImpaladMetrics, but I rather have 
single place to track this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 22:15:00 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10352/ 
DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 22:15:32 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 10: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 22:15:31 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 9: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21100/9/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/21100/9/be/src/service/impala-server.cc@1167
PS9, Line 1167: 
ImpaladMetrics::QUERY_LOG_EST_TOTAL_BYTES->Increment(record_size);
It would make sense to me for this to be tied to the server rather than a 
global. However that doesn't seem to be how everything else is handled in 
ImpaladMetrics, so :shrug:



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 22:09:35 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 9:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15396/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 21:42:05 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 8:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15395/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 21:41:46 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Jason Fehr (Code Review)
Jason Fehr has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 9: Code-Review+1

looks great!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 21:23:00 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21100/7/be/src/util/impalad-metrics.h
File be/src/util/impalad-metrics.h:

http://gerrit.cloudera.org:8080/#/c/21100/7/be/src/util/impalad-metrics.h@267
PS7, Line 267:   static const char* QUERY_LOG_EST_TOTAL_BYTES;
> Nit:  rename to match new metric name QUERY_LOG_EST_TOTAL_BYTES
Done


http://gerrit.cloudera.org:8080/#/c/21100/7/be/src/util/impalad-metrics.h@351
PS7, Line 351:   static IntGauge* QUERY_LOG_EST_TOTAL_BYTES;
> Nit:  rename to match new metric name QUERY_LOG_EST_TOTAL_BYTES
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 21:17:26 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Hello Daniel Becker, Jason Fehr, Michael Smith, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21100

to look at the new patch set (#9).

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..

IMPALA-12864: Deflake test_query_log_size_in_bytes.

test_query_log_size_in_bytes is flaky in exhaustive builds. The expected
QueryStateRecord is off by around 100KB per query, indicating that the
test query might be too complex and lead to variability in final query
profile that is being archived.

This patch simplifies the test query and relaxes the assertion. This
patch also adds QUERY_LOG_EST_TOTAL_BYTES metric (key name
"impala-server.query-log-est-total-bytes") that is validated as well in
test_query_log_size_in_bytes. query-state-record-test.cc is modified a
bit to resolve segmentation fault issue when building
unified-be-test-validated-executable target run_clang_tidy.sh.

Testing:
- Pass test_query_log_size_in_bytes in exhaustive build.

Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record-test.cc
M be/src/service/query-state-record.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M bin/run_clang_tidy.sh
M common/thrift/metrics.json
M tests/custom_cluster/test_web_pages.py
10 files changed, 55 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/21100/9
--
To view, visit http://gerrit.cloudera.org:8080/21100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21100/8/be/src/util/impalad-metrics.cc
File be/src/util/impalad-metrics.cc:

http://gerrit.cloudera.org:8080/#/c/21100/8/be/src/util/impalad-metrics.cc@348
PS8, Line 348:   QUERY_LOG_EST_TOTAL_BYTES = 
m->AddGauge(ImpaladMetricKeys::QUERY_LOG_EST_TOTAL_BYTES, 0);
line too long (91 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 21:15:53 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Hello Daniel Becker, Jason Fehr, Michael Smith, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21100

to look at the new patch set (#8).

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..

IMPALA-12864: Deflake test_query_log_size_in_bytes.

test_query_log_size_in_bytes is flaky in exhaustive builds. The expected
QueryStateRecord is off by around 100KB per query, indicating that the
test query might be too complex and lead to variability in final query
profile that is being archived.

This patch simplifies the test query and relaxes the assertion. This
patch also adds QUERY_LOG_EST_TOTAL_BYTES metric (key name
"impala-server.query-log-est-total-bytes") that is validated as well in
test_query_log_size_in_bytes. query-state-record-test.cc is modified a
bit to resolve segmentation fault issue when building
unified-be-test-validated-executable target run_clang_tidy.sh.

Testing:
- Pass test_query_log_size_in_bytes in exhaustive build.

Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record-test.cc
M be/src/service/query-state-record.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M bin/run_clang_tidy.sh
M common/thrift/metrics.json
M tests/custom_cluster/test_web_pages.py
10 files changed, 54 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/21100/8
--
To view, visit http://gerrit.cloudera.org:8080/21100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Jason Fehr (Code Review)
Jason Fehr has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21100/7/be/src/util/impalad-metrics.h
File be/src/util/impalad-metrics.h:

http://gerrit.cloudera.org:8080/#/c/21100/7/be/src/util/impalad-metrics.h@267
PS7, Line 267:   static const char* QUERY_LOG_EST_TOTAL_SIZE;
Nit:  rename to match new metric name QUERY_LOG_EST_TOTAL_BYTES


http://gerrit.cloudera.org:8080/#/c/21100/7/be/src/util/impalad-metrics.h@351
PS7, Line 351:   static IntGauge* QUERY_LOG_EST_TOTAL_SIZE;
Nit:  rename to match new metric name QUERY_LOG_EST_TOTAL_BYTES



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 21:11:15 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 7:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15394/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 21:04:10 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 30:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15393/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 30
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:54:18 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 29:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15392/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 29
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:50:37 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 7:

(2 comments)

ps7 also tweak run_clang_tidy.sh to grep make Error.

Dumped build output is so big (almost 1GB of text file).
It is helpful to summarize make Error in the beginning of tidylog.txt.

http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h@1137
PS5, Line 1137:   std::mutex query_log_lock_;
> Thinking about this more, it is good that the QUERY_LOG_EST_TOTAL_SIZE  met
Added comment at impalad-metrics.h


http://gerrit.cloudera.org:8080/#/c/21100/5/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/21100/5/common/thrift/metrics.json@2864
PS5, Line 2864: "key": "impala-server.query-log-est-total-bytes"
> Nit: The suffix "size" on the Impala metrics typically refers to a count of
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:43:58 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Hello Daniel Becker, Jason Fehr, Michael Smith, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21100

to look at the new patch set (#7).

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..

IMPALA-12864: Deflake test_query_log_size_in_bytes.

test_query_log_size_in_bytes is flaky in exhaustive builds. The expected
QueryStateRecord is off by around 100KB per query, indicating that the
test query might be too complex and lead to variability in final query
profile that is being archived.

This patch simplifies the test query and relaxes the assertion. This
patch also adds QUERY_LOG_EST_TOTAL_SIZE metric (key name
"impala-server.query-log-est-total-size") that is validated as well in
test_query_log_size_in_bytes. query-state-record-test.cc is modified a
bit to resolve segmentation fault issue when building
unified-be-test-validated-executable target run_clang_tidy.sh.

Testing:
- Pass test_query_log_size_in_bytes in exhaustive build.

Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record-test.cc
M be/src/service/query-state-record.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M bin/run_clang_tidy.sh
M common/thrift/metrics.json
M tests/custom_cluster/test_web_pages.py
10 files changed, 54 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/21100/7
--
To view, visit http://gerrit.cloudera.org:8080/21100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Jason Fehr (Code Review)
Hello Andrew Sherman, Riza Suminto, Michael Smith, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/20770

to look at the new patch set (#30).

Change subject: IMPALA-12426: Query History Table
..

IMPALA-12426: Query History Table

Adds the ability for users to specify that Impala will create and
maintain an internal Iceberg table that contains data about all
completed queries. This table is automatically created at startup by
each coordinator if it does not exist. Then, most completed queries are
queued in memory and flushed to the query history table at a set
interval (either minutes or number of records). Set, use, and show
queries are not written to this table. This commit leverages the
InternalServer class to maintain the query history table.

Ctest unit tests have been added to assert the various pieces of code.
New custom cluster tests have been added to assert the query history
table is properly populated with completed queries.

Negative testing consists of attempting sql injection attacks and
syntactically incorrect queries.

Impala built-in string functions benchmarks have been updated to include
the new built-in functions.

Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
---
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/service/CMakeLists.txt
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/internal-server.cc
M be/src/service/internal-server.h
A be/src/service/workload-management.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/BackendGflags.thrift
M common/thrift/metrics.json
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/CatalogBlacklistUtils.java
M tests/beeswax/impala_beeswax.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_connection.py
A tests/custom_cluster/test_query_log.py
A tests/util/assert_time.py
A tests/util/memory.py
A tests/util/retry.py
25 files changed, 3,010 insertions(+), 67 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/20770/30
--
To view, visit http://gerrit.cloudera.org:8080/20770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 30
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 6:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15391/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:31:27 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Jason Fehr (Code Review)
Jason Fehr has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h@1137
PS5, Line 1137:   std::mutex query_log_lock_;
> Agree. QUERY_LOG_EST_TOTAL_SIZE itself is already Atomic. Drop it from comm
Thinking about this more, it is good that the QUERY_LOG_EST_TOTAL_SIZE  metric 
is modified while this lock is held since that ensures this metric always 
matches the data stored in the query_log_est_sizes_ list.  Please consider 
mentioning that modifications of this metric are done while this lock is held.


http://gerrit.cloudera.org:8080/#/c/21100/5/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/21100/5/common/thrift/metrics.json@2864
PS5, Line 2864: "key": "impala-server.query-log-est-total-size"
Nit: The suffix "size" on the Impala metrics typically refers to a count of 
some sort and not to a specific number of bytes.  I suggest naming this metric 
"impala-server.query-log-est-total-bytes".



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:27:22 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20770 )

Change subject: IMPALA-12426: Query History Table
..


Patch Set 29:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/20770/29/tests/common/impala_connection.py
File tests/common/impala_connection.py:

http://gerrit.cloudera.org:8080/#/c/20770/29/tests/common/impala_connection.py@31
PS29, Line 31: from ImpalaService import ImpalaHiveServer2Service
flake8: F401 'ImpalaService.ImpalaHiveServer2Service' imported but unused


http://gerrit.cloudera.org:8080/#/c/20770/29/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/20770/29/tests/custom_cluster/test_query_log.py@490
PS29, Line 490: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/20770/29/tests/custom_cluster/test_query_log.py@491
PS29, Line 491: \
flake8: W605 invalid escape sequence '\)'


http://gerrit.cloudera.org:8080/#/c/20770/29/tests/custom_cluster/test_query_log.py@491
PS29, Line 491: \
flake8: W605 invalid escape sequence '\s'


http://gerrit.cloudera.org:8080/#/c/20770/29/tests/custom_cluster/test_query_log.py@491
PS29, Line 491: \
flake8: W605 invalid escape sequence '\S'


http://gerrit.cloudera.org:8080/#/c/20770/29/tests/util/memory.py
File tests/util/memory.py:

http://gerrit.cloudera.org:8080/#/c/20770/29/tests/util/memory.py@30
PS29, Line 30: def convert_to_bytes(mem_str, unit_combined=False, factor=1024):
flake8: E302 expected 2 blank lines, found 1



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 29
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:25:30 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Gabor Kaszab (Code Review)
Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/21099/2/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-delete.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-delete.test:

http://gerrit.cloudera.org:8080/#/c/21099/2/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-delete.test@421
PS2, Line 421: BUFFERED DELETE FROM ICEBERG 
[functional_parquet.iceberg_v2_partitioned_position_deletes-POSITION-DELETE]
I'm wondering if getting rid of DELETE FROM ICEBERG in this patch is how 
tightly related to the issue we are about to solve with this patch. For me it 
seems kind of a separate stuff at first sight.
If you say that this holds low or non-existing risk then I'm fine keeping the 
patch as is now, I just want to make sure that we won't introduce any further 
risks with fixing one issue. e.g. perf degradation with switching from 
DeleteSink to BufferedDeleteSink


http://gerrit.cloudera.org:8080/#/c/21099/2/tests/stress/test_update_stress.py
File tests/stress/test_update_stress.py:

http://gerrit.cloudera.org:8080/#/c/21099/2/tests/stress/test_update_stress.py@279
PS2, Line 279: # Prepare INSERT statement of 'num_rows' records.
nit: I meant to add it to a comment that this code prefers the INSERT in such a 
way that all the rows will be added in a single insert to have a single file 
(opposed to the similar part of the test above that adds a separate file for 
each row).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:24:51 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12426: Query History Table

2024-03-04 Thread Jason Fehr (Code Review)
Hello Andrew Sherman, Riza Suminto, Michael Smith, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/20770

to look at the new patch set (#29).

Change subject: IMPALA-12426: Query History Table
..

IMPALA-12426: Query History Table

Adds the ability for users to specify that Impala will create and
maintain an internal Iceberg table that contains data about all
completed queries. This table is automatically created at startup by
each coordinator if it does not exist. Then, most completed queries are
queued in memory and flushed to the query history table at a set
interval (either minutes or number of records). Set, use, and show
queries are not written to this table. This commit leverages the
InternalServer class to maintain the query history table.

Ctest unit tests have been added to assert the various pieces of code.
New custom cluster tests have been added to assert the query history
table is properly populated with completed queries.

Negative testing consists of attempting sql injection attacks and
syntactically incorrect queries.

Impala built-in string functions benchmarks have been updated to include
the new built-in functions.

Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
---
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/service/CMakeLists.txt
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/internal-server.cc
M be/src/service/internal-server.h
A be/src/service/workload-management.cc
M be/src/util/backend-gflag-util.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/BackendGflags.thrift
M common/thrift/metrics.json
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/CatalogBlacklistUtils.java
M tests/beeswax/impala_beeswax.py
M tests/common/custom_cluster_test_suite.py
M tests/common/impala_connection.py
A tests/custom_cluster/test_query_log.py
A tests/util/assert_time.py
A tests/util/memory.py
A tests/util/retry.py
25 files changed, 3,010 insertions(+), 67 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/20770/29
--
To view, visit http://gerrit.cloudera.org:8080/20770
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844
Gerrit-Change-Number: 20770
Gerrit-PatchSet: 29
Gerrit-Owner: Jason Fehr 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG@14
PS5, Line 14: This patch simplifies the test query and relaxes the assertion. 
This
> Nit: and relaxes the assertion
Done


http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG@15
PS5, Line 15: patch also adds QUERY_LOG_EST_TOTAL_SIZE metric (key name
> Please mention the name of the new "impala-server.query-log-est-total-size"
Done


http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h@1137
PS5, Line 1137:   std::mutex query_log_lock_;
> The lock does not protect the QUERY_LOG_EST_TOTAL_SIZE when it's being read
Agree. QUERY_LOG_EST_TOTAL_SIZE itself is already Atomic. Drop it from comment.


http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py
File tests/custom_cluster/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py@153
PS5, Line 153:   @CustomClusterTestSuite.with_args(
> Nit: Since the (40 * 1024) calculation is used in two places, it would elim
Done


http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py@178
PS5, Line 178:   # 'should_exist'
> The above code can be replaced with:
Done. I'd like to keep the assertion separate so it is clear which expression 
is failed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 20:07:49 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Hello Daniel Becker, Jason Fehr, Michael Smith, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21100

to look at the new patch set (#6).

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..

IMPALA-12864: Deflake test_query_log_size_in_bytes.

test_query_log_size_in_bytes is flaky in exhaustive builds. The expected
QueryStateRecord is off by around 100KB per query, indicating that the
test query might be too complex and lead to variability in final query
profile that is being archived.

This patch simplifies the test query and relaxes the assertion. This
patch also adds QUERY_LOG_EST_TOTAL_SIZE metric (key name
"impala-server.query-log-est-total-size") that is validated as well in
test_query_log_size_in_bytes. query-state-record-test.cc is modified a
bit to resolve segmentation fault issue when building
unified-be-test-validated-executable target run_clang_tidy.sh.

Testing:
- Pass test_query_log_size_in_bytes in exhaustive build.

Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record-test.cc
M be/src/service/query-state-record.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_web_pages.py
9 files changed, 50 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/21100/6
--
To view, visit http://gerrit.cloudera.org:8080/21100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Jason Fehr (Code Review)
Jason Fehr has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG@14
PS5, Line 14: This patch simplifies the test query add relaxes the assertion. 
This
Nit: and relaxes the assertion


http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG@15
PS5, Line 15: patch also adds QUERY_LOG_EST_TOTAL_SIZE metric that is validated 
as
Please mention the name of the new "impala-server.query-log-est-total-size" 
metric as well.


http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h@1137
PS5, Line 1137:   /// QUERY_LOG_EST_TOTAL_SIZE metric.
The lock does not protect the QUERY_LOG_EST_TOTAL_SIZE when it's being read by 
the Impala http server.  I don't think that's an issue since the metric is 
initialized once, and that initialization happens long before the Impala http 
server can access the metric.  As far as I know, the only potential issue is 
the impala-server.query-log-est-total-size metric may be slightly mis-reported, 
but that is ok since the Impala http server always lags behind the query_log_ 
anyways.


http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/query-state-record.h
File be/src/service/query-state-record.h:

http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/query-state-record.h@181
PS5, Line 181:   int64_t peak_memory_usage = 0;
Thanks for fixing!


http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py
File tests/custom_cluster/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py@153
PS5, Line 153:   impalad_args="--query_log_size_in_bytes=" + str(40 * 1024)
Nit: Since the (40 * 1024) calculation is used in two places, it would 
eliminate the repetition to declare a variable above this test function as use 
that variable in both places.


http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py@178
PS5, Line 178: assert log_metric["value"] <= 40 * 1024, log_metric
The above code can be replaced with:
impalad = self.cluster.get_first_impalad()
assert 0 < 
impalad.service.get_metric_value("impala-server.query-log-est-total-size") < 40 
* 1024



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 19:43:04 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Anonymous Coward (Code Review)
k.venureddy2...@gmail.com has abandoned this change. ( 
http://gerrit.cloudera.org:8080/21106 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Abandoned

duplicate of another commit
--
To view, visit http://gerrit.cloudera.org:8080/21106
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib11c554f5692e4fb8a4c63ef5ba8f8ae0e787826
Gerrit-Change-Number: 21106
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10351/ 
DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 04 Mar 2024 19:42:10 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10350/ 
DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 18:58:43 +
Gerrit-HasComments: No


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 9:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15390/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 04 Mar 2024 18:35:48 +
Gerrit-HasComments: No


[native-toolchain-CR] IMPALA-12863: Fix toolchain container builds for CentOS 7, SLES 12 & 15

2024-03-04 Thread Michael Smith (Code Review)
Michael Smith has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21091 )

Change subject: IMPALA-12863: Fix toolchain container builds for CentOS 7, SLES 
12 & 15
..


Patch Set 3: Code-Review+1


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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3e730abb6667bf00735ea62c35377591b68452ce
Gerrit-Change-Number: 21091
Gerrit-PatchSet: 3
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Smith 
Gerrit-Comment-Date: Mon, 04 Mar 2024 18:25:56 +
Gerrit-HasComments: No


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Anonymous Coward (Code Review)
Hello Quanlong Huang, Sai Hemanth Gantasala, Csaba Ringhofer, Impala Public 
Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21065

to look at the new patch set (#9).

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..

[WIP]IMPALA-12832: Implicit invalidate metadata on event failures

At present, failure in event processing needs manual invalidate
metadata. This patch implicitly invalidates the table upon failures
in processing of table events with new 'process_event_failure' flag.
And a new 'auto_global_invalidate_metadata' flag is added to global
invalidate metadata automatically when event processor goes to
non-active state.

Note: Also introduced a config
'inject_process_event_failure_event_types' for automated tests to
simulate event processor failures. This config is used to specify what
event types can be intentionally failed. This config should only be
used for testing purpose.

Testing:
- Added end-to-end tests to mimic failures in event processor and verified
that event processor is active
- Passed FE tests

Co-Authored-by: Sai Hemanth Gantasala 

Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
---
M be/src/catalog/catalog-server.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M tests/common/impala_test_suite.py
A tests/metadata/__init__.py
M tests/metadata/test_event_processing.py
A tests/metadata/test_event_processing_base.py
A tests/metadata/test_event_processing_error.py
M tests/util/event_processor_utils.py
15 files changed, 664 insertions(+), 304 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/21065/9
--
To view, visit http://gerrit.cloudera.org:8080/21065
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 9:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/21065/9/tests/metadata/__init__.py
File tests/metadata/__init__.py:

http://gerrit.cloudera.org:8080/#/c/21065/9/tests/metadata/__init__.py@1
PS9, Line 1:
flake8: W292 no newline at end of file


http://gerrit.cloudera.org:8080/#/c/21065/9/tests/metadata/test_event_processing_base.py
File tests/metadata/test_event_processing_base.py:

http://gerrit.cloudera.org:8080/#/c/21065/9/tests/metadata/test_event_processing_base.py@348
PS9, Line 348:
flake8: W292 no newline at end of file


http://gerrit.cloudera.org:8080/#/c/21065/9/tests/metadata/test_event_processing_error.py
File tests/metadata/test_event_processing_error.py:

http://gerrit.cloudera.org:8080/#/c/21065/9/tests/metadata/test_event_processing_error.py@30
PS9, Line 30: @SkipIfCatalogV2.hms_event_polling_disabled()
flake8: E302 expected 2 blank lines, found 1


http://gerrit.cloudera.org:8080/#/c/21065/9/tests/metadata/test_event_processing_error.py@64
PS9, Line 64:
flake8: W292 no newline at end of file



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia67fc04c995802d3b6b56f79564bf0954b012c6c
Gerrit-Change-Number: 21065
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 04 Mar 2024 18:09:52 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 2:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15389/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 18:05:14 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-10949: (Addendum) Improve batching logic of events

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21032 )

Change subject: IMPALA-10949: (Addendum) Improve batching logic of events
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21032/1/tests/custom_cluster/test_events_custom_configs.py
File tests/custom_cluster/test_events_custom_configs.py:

http://gerrit.cloudera.org:8080/#/c/21032/1/tests/custom_cluster/test_events_custom_configs.py@411
PS1, Line 411: self.run_stmt_in_hive(
 :   "analyze table {}.{} compute 
statistics".format(unique_database, test_batch_table))
 : self.client.execute("refresh 
{0}.{1}".format(unique_database, test_batch_table))
Is it possible that the event poll and processing complete faster than catalogd 
handling the REFRESH query?

If it possible, then a better fix is to inject sleep in Event Processor to hold 
processing event batch after poll, so that REFRESH happen before event batch 
processing. This will require exposing debug_action flag to Java code via 
BackendConfig.java.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c6d0c3e3449f98a41058d0186208f17eb91cd0d
Gerrit-Change-Number: 21032
Gerrit-PatchSet: 1
Gerrit-Owner: Sai Hemanth Gantasala 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 04 Mar 2024 17:44:41 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Zoltan Borok-Nagy (Code Review)
Hello Daniel Becker, Gabor Kaszab, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21099

to look at the new patch set (#2).

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..

IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

We must invoke validateDataFilesExist for RowDelta operations (DELETE/
UPDATE/MERGE). Without this a concurrent RewriteFiles (compaction) and
RowDelta can corrupt a table.

IcebergBufferedDeleteSink now also collects the filenames of the data
files that are referenced in the position delete files. It adds them to
the DML exec state which is then collected by the Coordinator. The
Coordinator passes the file paths to CatalogD which executes Iceberg's
RowDelta operation and now invokes validateDataFilesExist() with the
file paths. Additionally it also invokes validateDeletedFiles().

This patch set also resolves IMPALA-12640 which is about replacing
IcebergDeleteSink with IcebergBufferedDeleteSink, as from now on
we use the buffered version for all DML operations that write
position delete files.

Testing:
 * adds new stress test with DELETE + UPDATE + OPTIMIZE

Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
---
M be/src/exec/CMakeLists.txt
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/iceberg-delete-sink-config.cc
D be/src/exec/iceberg-delete-sink.cc
D be/src/exec/iceberg-delete-sink.h
M be/src/exec/multi-table-sink.cc
M be/src/runtime/dml-exec-state.cc
M be/src/runtime/dml-exec-state.h
M be/src/service/client-request-state.cc
M common/protobuf/control_service.proto
M common/thrift/CatalogService.thrift
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java
M fe/src/main/java/org/apache/impala/planner/IcebergBufferedDeleteSink.java
D fe/src/main/java/org/apache/impala/planner/IcebergDeleteSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-delete.test
M tests/stress/test_update_stress.py
20 files changed, 163 insertions(+), 580 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/21099/2
--
To view, visit http://gerrit.cloudera.org:8080/21099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 5:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15388/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 16:20:02 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Daniel Becker (Code Review)
Daniel Becker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 5: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py
File tests/custom_cluster/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@180
PS4, Line 180: he query page only contai
> This one is scraped from /queries page, while the one in L166 is scraped fr
You're right, thanks for clarifying it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 16:10:19 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Riza Suminto has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 5:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@9
PS4, Line 9: build
> Nit: builds.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@14
PS4, Line 14: d rel
> Nit: relaxes.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@14
PS4, Line 14: simplifi
> Nit: simplifies.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@15
PS4, Line 15:  al
> Nit: adds.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4/be/src/util/impalad-metrics.h
File be/src/util/impalad-metrics.h:

http://gerrit.cloudera.org:8080/#/c/21100/4/be/src/util/impalad-metrics.h@264
PS4, Line 264: that
> Nit: that are currently.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/21100/4/common/thrift/metrics.json@2857
PS4, Line 2857: that are curre
> Nit: that are currently.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py
File tests/custom_cluster/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@173
PS4, Line 173: log_metric = metric
> Could add a break here. Or do we reach this multiple times and we need the
Done. There is only 1 entry.


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@174
PS4, Line 174:
> Before writing the response text we could add some info about what's coming
Done


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178
PS4, Line 178: ric
> Nit: the test queries.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178
PS4, Line 178: ric["
> Nit: the query page.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178
PS4, Line 178: 4, log
> Nit: a subset.
Done


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@180
PS4, Line 180: he query page only contai
> The text is already parsed into JSON on L166, we could move this variable t
This one is scraped from /queries page, while the one in L166 is scraped from 
/metrics page.
Renamed the variable to clarify.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 15:56:36 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Riza Suminto (Code Review)
Hello Daniel Becker, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/21100

to look at the new patch set (#5).

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..

IMPALA-12864: Deflake test_query_log_size_in_bytes.

test_query_log_size_in_bytes is flaky in exhaustive builds. The expected
QueryStateRecord is off by around 100KB per query, indicating that the
test query might be too complex and lead to variability in final query
profile that is being archived.

This patch simplifies the test query add relaxes the assertion. This
patch also adds QUERY_LOG_EST_TOTAL_SIZE metric that is validated as
well in test_query_log_size_in_bytes. query-state-record-test.cc is
modified a bit to resolve segmentation fault issue when building
unified-be-test-validated-executable target run_clang_tidy.sh.

Testing:
- Pass test_query_log_size_in_bytes in exhaustive build.

Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-state-record-test.cc
M be/src/service/query-state-record.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_web_pages.py
9 files changed, 60 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/21100/5
--
To view, visit http://gerrit.cloudera.org:8080/21100
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 


[Impala-ASF-CR] IMPALA-12864: Deflake test query log size in bytes.

2024-03-04 Thread Daniel Becker (Code Review)
Daniel Becker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21100 )

Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes.
..


Patch Set 4:

(12 comments)

Thanks Riza!

http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@9
PS4, Line 9: build
Nit: builds.


http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@14
PS4, Line 14: relax
Nit: relaxes.


http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@14
PS4, Line 14: simplify
Nit: simplifies.


http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@15
PS4, Line 15: add
Nit: adds.


http://gerrit.cloudera.org:8080/#/c/21100/4/be/src/util/impalad-metrics.h
File be/src/util/impalad-metrics.h:

http://gerrit.cloudera.org:8080/#/c/21100/4/be/src/util/impalad-metrics.h@264
PS4, Line 264: that
Nit: that are currently.


http://gerrit.cloudera.org:8080/#/c/21100/4/common/thrift/metrics.json
File common/thrift/metrics.json:

http://gerrit.cloudera.org:8080/#/c/21100/4/common/thrift/metrics.json@2857
PS4, Line 2857: that currently
Nit: that are currently.


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py
File tests/custom_cluster/test_web_pages.py:

http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@173
PS4, Line 173: log_metric = metric
Could add a break here. Or do we reach this multiple times and we need the last 
one?


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@174
PS4, Line 174: response.text
Before writing the response text we could add some info about what's coming, 
for example "Metrics: " or "Metrics received from WebUI: ".


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178
PS4, Line 178: test
Nit: the test queries.


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178
PS4, Line 178: query
Nit: the query page.


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178
PS4, Line 178: subset
Nit: a subset.


http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@180
PS4, Line 180: json.loads(response.text)
The text is already parsed into JSON on L166, we could move this variable there 
so we don't have to parse the text twice.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25
Gerrit-Change-Number: 21100
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Mon, 04 Mar 2024 15:02:43 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 1: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10349/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 14:26:57 +
Gerrit-HasComments: No


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21106 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 1:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15387/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib11c554f5692e4fb8a4c63ef5ba8f8ae0e787826
Gerrit-Change-Number: 21106
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 13:48:04 +
Gerrit-HasComments: No


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Anonymous Coward (Code Review)
k.venureddy2...@gmail.com has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21065 )

Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..


Patch Set 8:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/21065/8/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/21065/8/be/src/catalog/catalog-server.cc@174
PS8, Line 174: DEFINE_string
> nit: define this as hidden using DEFINE_string_hidden()
Done


http://gerrit.cloudera.org:8080/#/c/21065/3/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/21065/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2753
PS3, Line 2753: etTable(
> I am not sure if it is really needed to deal with rename. Generally events
Agreed. Not invalidating tables for create/drop/alter table rename events.


http://gerrit.cloudera.org:8080/#/c/21065/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/21065/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1292
PS7, Line 1292: e);
  : catalog_.invalidateTableIfExists(dbName_, tblName_);
  :   }
> nit: we can use warnLog()
Done


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:

http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@a2811
PS8, Line 2811:
> Could you explain why we remove this?
Have removed it from this gerrit. It is fixed with 
https://issues.apache.org/jira/browse/IMPALA-12851 now.


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@677
PS8, Line 677: BackendConfig.INSTANCE.getProcessEventFailureEventTypes()
> nit: this can also be replaced with catalog_.getFailureEventsForTesting()
We don't need this check anymore. Have directly call contains() instead.


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@679
PS8, Line 679: Double
> nit: use primitive type 'double'
Done


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1368
PS8, Line 1368:   return false;
> Maybe we can use CatalogServiceCatalog.invalidateDb() here. But it seems lo
Have added a todo for now


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1787
PS8, Line 1787: tableBefore_.getDbName(), 
tableBefore_.getTableName());
> Please add a comment saying the new table will be invalidated in MetastoreT
Not doing invalidateTableIfExists in case of rename table. Have added a comment 
for it now.


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2902
PS8, Line 2902: is not processed by then. So try to get the table again 
here if it is null */
> This seems fixing an existing bug. Can we split this into another patch?
Done


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@3105
PS8, Line 3105: tableWriteId.getDbName(), 
tableWriteId.getTblName());
> I think we should deduplicate the table names before invalidating them. The
Done


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1006
PS8, Line 1006:   try {
> Can we add a log for this?
Done


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1188
PS8, Line 1188:   if (!event.onFailure(e)) { throw e; }
> Can we add a log when throwing the exception? e.g. "Unable to handle failur
Done


http://gerrit.cloudera.org:8080/#/c/21065/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1190
PS8, Line 1190:   LOG.error(event.debugString("Failed to handle 
event processing failure"));
> The invalidation in onFailure() could still fail by bugs like IMPALA-12831.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Ge

[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Daniel Becker (Code Review)
Daniel Becker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 13:23:46 +
Gerrit-HasComments: No


[Impala-ASF-CR] [WIP]IMPALA-12832: Implicit invalidate metadata on event failures

2024-03-04 Thread Anonymous Coward (Code Review)
k.venureddy2...@gmail.com has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21106


Change subject: [WIP]IMPALA-12832: Implicit invalidate metadata on event 
failures
..

[WIP]IMPALA-12832: Implicit invalidate metadata on event failures

At present, failure in event processing needs manual invalidate
metadata. This patch implicitly invalidates the table upon failures
in processing of table events with new 'process_event_failure' flag.
And a new 'auto_global_invalidate_metadata' flag is added to global
invalidate metadata automatically when event processor goes to
non-active state.

Note: Also introduced a config
'inject_process_event_failure_event_types' for automated tests to
simulate event processor failures. This config is used to specify what
event types can be intentionally failed. This config should only be
used for testing purpose.

Testing:
- Added end-to-end tests to mimic failures in event processor and verified
that event processor is active
- Passed FE tests

Change-Id: Ib11c554f5692e4fb8a4c63ef5ba8f8ae0e787826
---
M be/src/catalog/catalog-server.cc
M fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M 
fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
6 files changed, 52 insertions(+), 59 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/21106/1
--
To view, visit http://gerrit.cloudera.org:8080/21106
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib11c554f5692e4fb8a4c63ef5ba8f8ae0e787826
Gerrit-Change-Number: 21106
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward 


[Impala-ASF-CR] IMPALA-12845: Crash with DESCRIBE on a complex type from an Iceberg table

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21105 )

Change subject: IMPALA-12845: Crash with DESCRIBE on a complex type from an 
Iceberg table
..


Patch Set 1:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15386/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eda21a41167cc1fda183aa16fd6276a6a16f5d3
Gerrit-Change-Number: 21105
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Becker 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Noemi Pap-Takacs 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Mon, 04 Mar 2024 13:01:36 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12845: Crash with DESCRIBE on a complex type from an Iceberg table

2024-03-04 Thread Daniel Becker (Code Review)
Daniel Becker has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21105


Change subject: IMPALA-12845: Crash with DESCRIBE on a complex type from an 
Iceberg table
..

IMPALA-12845: Crash with DESCRIBE on a complex type from an Iceberg table

A DESCRIBE statement on a complex column contained in an Iceberg table
runs into a DCHECK and crashes Impala. An example with an array:

  describe functional_parquet.iceberg_resolution_test_external.phone

Note that this also happens with Iceberg metadata tables, for example:
  describe functional_parquet.iceberg_query_metadata.\
  entries.readable_metrics;

With non-Iceberg tables there is no error.

The problem is that for Iceberg tables, the DESCRIBE statement returns
four columns: "name", "type", "comment" and "nullable" (only Iceberg and
Kudu tables have "nullable"). However, the DESCRIBE statement response
for complex types only contains the first three columns as they are
always nullable. But as the table is an Iceberg table, the 'metadata_'
field of HS2ColumnarResultSet is still populated with four columns.

The DCHECK in HS2ColumnarResultSet::AddOneRow() expects the number of
columns to be the same in the DESCRIBE statement response and the
'metadata_' field.

This commit solves the problem by only adding the "nullable" column to
the 'metadata_' field if the target of the DESCRIBE statement is a
table, not a complex type.

Note that Kudu tables do not support complex types so this issue does
not arise there.

This change also addresses a minor issue: DescribeTableStmt::analyze()
did not check whether the statement was already analyzed and after
analysis did not set the 'analyzer_' field which would indicate that
analysis had already been done. This is now corrected.

Testing:
 - added tests in describe-path.test for arrays, maps and structs from
   regular Iceberg tables and metadata tables.

Change-Id: I5eda21a41167cc1fda183aa16fd6276a6a16f5d3
---
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M testdata/workloads/functional-query/queries/QueryTest/describe-path.test
3 files changed, 91 insertions(+), 14 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/21105/1
--
To view, visit http://gerrit.cloudera.org:8080/21105
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5eda21a41167cc1fda183aa16fd6276a6a16f5d3
Gerrit-Change-Number: 21105
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Becker 


[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Gabor Kaszab (Code Review)
Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 1:

(7 comments)

great job with this quick fix, Zoltan! I added some nits but in general the 
patch is good.

http://gerrit.cloudera.org:8080/#/c/21099/1/be/src/runtime/dml-exec-state.h
File be/src/runtime/dml-exec-state.h:

http://gerrit.cloudera.org:8080/#/c/21099/1/be/src/runtime/dml-exec-state.h@135
PS1, Line 135:  const std::vector& 
ReferencedDataFilesByPositionDeletes() {
since it returns const ref, can the function also be const?


http://gerrit.cloudera.org:8080/#/c/21099/1/be/src/runtime/dml-exec-state.h@170
PS1, Line 170: referenced_data_files_by_position_deletes_
nit: data_files_referenced_by_position_deletes


http://gerrit.cloudera.org:8080/#/c/21099/1/common/protobuf/control_service.proto
File common/protobuf/control_service.proto:

http://gerrit.cloudera.org:8080/#/c/21099/1/common/protobuf/control_service.proto@115
PS1, Line 115: referenced_data_files_by_position_deletes
nit: data_files_referenced_by_position_deletes?


http://gerrit.cloudera.org:8080/#/c/21099/1/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:

http://gerrit.cloudera.org:8080/#/c/21099/1/common/thrift/CatalogService.thrift@249
PS1, Line 249: referenced_data_files_by_position_deletes
nit: data_files_referenced_by_position_deletes maybe?


http://gerrit.cloudera.org:8080/#/c/21099/1/tests/stress/test_update_stress.py
File tests/stress/test_update_stress.py:

http://gerrit.cloudera.org:8080/#/c/21099/1/tests/stress/test_update_stress.py@269
PS1, Line 269: test_concurrent_deletes_and_updates
nit: this table name omits optimize. test_concurrent_write_and_optimize?


http://gerrit.cloudera.org:8080/#/c/21099/1/tests/stress/test_update_stress.py@277
PS1, Line 277: for i in range(num_rows):
Could you add a comment that here you prepare the INSERT query so that these 
rows can be put into the same file?


http://gerrit.cloudera.org:8080/#/c/21099/1/tests/stress/test_update_stress.py@283
PS1, Line 283: flag
I know we've been using this variable name for a while now, but I feel that the 
name doesn't suggest anything what it is used for. Can we rename this 
'all_rows_deleted' or such?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 12:15:41 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-12860: Invoke validateDataFilesExist for RowDelta operations

2024-03-04 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21099 )

Change subject: IMPALA-12860: Invoke validateDataFilesExist for RowDelta 
operations
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10349/ 
DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4869eb863ff0afe8f691ccf2fd681a92d36b405c
Gerrit-Change-Number: 21099
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Mon, 04 Mar 2024 09:47:07 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-12855: Fix NPE in firing RELOAD events when the partition doesn't exist

2024-03-04 Thread Quanlong Huang (Code Review)
Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/21096 )

Change subject: IMPALA-12855: Fix NPE in firing RELOAD events when the 
partition doesn't exist
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21096/1/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/21096/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6799
PS1, Line 6799: List eventId
> Ah, sorry to misunderstood this. That's really a problem! So we shouldn't u
A good news is that this only happens when --enable_skipping_older_events is 
true and it's false by default:
https://github.com/apache/impala/blob/784971c018c2dc44c53d7c0f366ad49cd8681ac6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java#L1199

Filed IMPALA-12865 to track this issue.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I01af3624bf7cf5cd69935cffa28d54f6a6807504
Gerrit-Change-Number: 21096
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Sai Hemanth Gantasala 
Gerrit-Comment-Date: Mon, 04 Mar 2024 08:23:13 +
Gerrit-HasComments: Yes