[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

2024-04-26 Thread Michael Smith (Code Review)
Michael Smith has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/21351 )

Change subject: IMPALA-13012: Lower default query_log_max_queued
..

IMPALA-13012: Lower default query_log_max_queued

Sets the query_log_max_queued default such that

  query_log_max_queued * num_columns(49) < statement_expression_limit

to avoid triggering e.g.

  AnalysisException: Exceeded the statement expression limit (25)
  Statement has 370039 expressions.

Also increases statement_expression_limit for insertion to avoid an
error if query_log_max_queued is changed.

Logs time taken to write to the queries table for help with debugging
and adds histogram "impala-server.completed-queries.write-durations".

Fixes InternalServer so it uses 'default_query_options'.

Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Reviewed-on: http://gerrit.cloudera.org:8080/21351
Reviewed-by: Michael Smith 
Tested-by: Michael Smith 
Reviewed-by: Riza Suminto 
---
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
M be/src/service/query-options.cc
M be/src/service/query-options.h
M be/src/service/workload-management-flags.cc
M be/src/service/workload-management.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/SystemTables.thrift
M common/thrift/metrics.json
M tests/custom_cluster/test_query_log.py
13 files changed, 115 insertions(+), 71 deletions(-)

Approvals:
  Michael Smith: Looks good to me, but someone else must approve; Verified
  Riza Suminto: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 9
Gerrit-Owner: Michael Smith 
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-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 8: Code-Review+2

Did another pass. Looks OK to go. Changed my vote to +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 26 Apr 2024 23:45:13 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 8: Verified+1 Code-Review+1

Carry +1s after merging parent.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 26 Apr 2024 23:24:36 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 26 Apr 2024 23:24:02 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 7: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 26 Apr 2024 19:09:16 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 7:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 26 Apr 2024 18:15:20 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 6: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 26 Apr 2024 00:45:23 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 6:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/16031/ : 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/21351
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Fri, 26 Apr 2024 00:29:37 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

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

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

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..

IMPALA-13012: Lower default query_log_max_queued

Sets the query_log_max_queued default such that

  query_log_max_queued * num_columns(49) < statement_expression_limit

to avoid triggering e.g.

  AnalysisException: Exceeded the statement expression limit (25)
  Statement has 370039 expressions.

Also increases statement_expression_limit for insertion to avoid an
error if query_log_max_queued is changed.

Logs time taken to write to the queries table for help with debugging
and adds histogram "impala-server.completed-queries.write-durations".

Fixes InternalServer so it uses 'default_query_options'.

Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
---
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
M be/src/service/query-options.cc
M be/src/service/query-options.h
M be/src/service/workload-management-flags.cc
M be/src/service/workload-management.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/SystemTables.thrift
M common/thrift/metrics.json
M tests/custom_cluster/test_query_log.py
13 files changed, 115 insertions(+), 71 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith 
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-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 5:

Build Failed

https://jenkins.impala.io/job/gerrit-code-review-checks/16026/ : Initial code 
review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 23:42:50 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 5: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 23:32:58 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

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

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

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..

IMPALA-13012: Lower default query_log_max_queued

Sets the query_log_max_queued default such that

  query_log_max_queued * num_columns(49) < statement_expression_limit

to avoid triggering e.g.

  AnalysisException: Exceeded the statement expression limit (25)
  Statement has 370039 expressions.

Also increases statement_expression_limit for insertion to avoid an
error if query_log_max_queued is changed.

Logs time taken to write to the queries table for help with debugging
and adds histogram "impala-server.completed-queries.write-durations".

Fixes InternalServer so it uses 'default_query_options'.

Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
---
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
M be/src/service/query-options.cc
M be/src/service/query-options.h
M be/src/service/workload-management-flags.cc
M be/src/service/workload-management.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/SystemTables.thrift
M common/thrift/metrics.json
M tests/custom_cluster/test_query_log.py
13 files changed, 115 insertions(+), 71 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith 
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-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 4:

Build Failed

https://jenkins.impala.io/job/gerrit-code-review-checks/16025/ : Initial code 
review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 22:54:27 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 4: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21351/4/be/src/service/internal-server.h
File be/src/service/internal-server.h:

http://gerrit.cloudera.org:8080/#/c/21351/4/be/src/service/internal-server.h@62
PS4, Line 62: std::map
nit: This can be std::map;
TImpalaQueryOptions::type is defined in 
./be/generated-sources/gen-cpp/ImpalaService_types.h

Enum to String can be done through _TImpalaQueryOptions_VALUES_TO_NAMES.
But Enum to String to Enum again in SetQueryOption() might be redundant.
Might be a good idea to make an overload function:

Status SetQueryOption(const TImpalaQueryOptions::type& key, const std::string& 
value,
TQueryOptions* query_options, QueryOptionsMask* set_query_options_mask);



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 22:47:18 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 3:

Build Failed

https://jenkins.impala.io/job/gerrit-code-review-checks/16024/ : Initial code 
review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 22:41:13 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21351/3/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/21351/3/tests/custom_cluster/test_query_log.py@314
PS3, Line 314: l
> flake8: E122 continuation line missing indentation or outdented
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 22:33:23 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

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

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

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..

IMPALA-13012: Lower default query_log_max_queued

Sets the query_log_max_queued default such that

  query_log_max_queued * num_columns(49) < statement_expression_limit

to avoid triggering e.g.

  AnalysisException: Exceeded the statement expression limit (25)
  Statement has 370039 expressions.

Also increases statement_expression_limit for insertion to avoid an
error if query_log_max_queued is changed.

Logs time taken to write to the queries table for help with debugging
and adds histogram "impala-server.completed-queries.write-durations".

Fixes InternalServer so it uses 'default_query_options'.

Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
---
M be/src/service/impala-server.h
M be/src/service/internal-server.cc
M be/src/service/internal-server.h
M be/src/service/workload-management-flags.cc
M be/src/service/workload-management.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/SystemTables.thrift
M common/thrift/metrics.json
M tests/custom_cluster/test_query_log.py
10 files changed, 88 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/21351/4
--
To view, visit http://gerrit.cloudera.org:8080/21351
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith 
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-13012: Lower default query log max queued

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

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

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

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..

IMPALA-13012: Lower default query_log_max_queued

Sets the query_log_max_queued default such that

  query_log_max_queued * num_columns(49) < statement_expression_limit

to avoid triggering e.g.

  AnalysisException: Exceeded the statement expression limit (25)
  Statement has 370039 expressions.

Also increases statement_expression_limit for insertion to avoid an
error if query_log_max_queued is changed.

Logs time taken to write to the queries table for help with debugging
and adds histogram "impala-server.completed-queries.write-durations".

Fixes InternalServer so it uses 'default_query_options'.

Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
---
M be/src/service/impala-server.h
M be/src/service/internal-server.cc
M be/src/service/internal-server.h
M be/src/service/workload-management-flags.cc
M be/src/service/workload-management.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/SystemTables.thrift
M common/thrift/metrics.json
M tests/custom_cluster/test_query_log.py
10 files changed, 88 insertions(+), 49 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith 
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-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/21351/3/tests/custom_cluster/test_query_log.py
File tests/custom_cluster/test_query_log.py:

http://gerrit.cloudera.org:8080/#/c/21351/3/tests/custom_cluster/test_query_log.py@314
PS3, Line 314: (
flake8: E122 continuation line missing indentation or outdented



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 22:23:30 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@441
PS2, Line 441: "gather_time=" << PrettyPrinter::Print(gather_time, 
TUnit::TIME_NS) << " "
 :   "exec_time=" << PrettyPrinter::Print(exec_time, 
TUnit::TIME_NS);
> Hmm, what combination. Could go minimal with one histogram for all gather+e
We can start with minimal. I'm guessing gather is much quicker than exec, since 
the latter deal with writing new parquet and iceberg metadata.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 19:09:06 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@441
PS2, Line 441: "gather_time=" << PrettyPrinter::Print(gather_time, 
TUnit::TIME_NS) << " "
 :   "exec_time=" << PrettyPrinter::Print(exec_time, 
TUnit::TIME_NS);
> Yeah, a histogram for these would be useful. I'll add it.
Hmm, what combination. Could go minimal with one histogram for all gather+exec 
time (with logs to look at the details), or maximal tracking gather and exec 
separately across succeeded or failed queries.

My instinct is minimal to make it easier to monitor, and rely on logs for more 
detail.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 18:53:46 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@428
PS2, Line 428: TQueryTableColumn::TABLES_QUERIED + 1
> Mostly indentation, I switched to using continue more to reduce nesting. I
Removed the indentation changes, and switched to testing size of 
TQueryTableColumn directly.


http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@441
PS2, Line 441: "gather_time=" << PrettyPrinter::Print(gather_time, 
TUnit::TIME_NS) << " "
 :   "exec_time=" << PrettyPrinter::Print(exec_time, 
TUnit::TIME_NS);
> ImpaladMetrics, like ImpaladMetrics::COMPLETED_QUERIES_QUEUED and ImpaladMe
Yeah, a histogram for these would be useful. I'll add it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 18:43:13 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@441
PS2, Line 441: "gather_time=" << PrettyPrinter::Print(gather_time, 
TUnit::TIME_NS) << " "
 :   "exec_time=" << PrettyPrinter::Print(exec_time, 
TUnit::TIME_NS);
> Metric? I'm not sure I follow.
ImpaladMetrics, like ImpaladMetrics::COMPLETED_QUERIES_QUEUED and 
ImpaladMetrics::COMPLETED_QUERIES_FAIL below, but maybe a histogram kind.

https://github.com/apache/impala/blob/f620e5d5c0bbdb0fd97bac31c7b7439cd13c6d08/be/src/util/impalad-metrics.h#L383-L385



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Thu, 25 Apr 2024 00:00:49 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management-flags.cc
File be/src/service/workload-management-flags.cc:

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management-flags.cc@82
PS2, Line 82: DEFINE_int32(query_log_max_queued, 5000
> Put a comment to update the default value if TQueryTableColumn change.
Not strictly necessary. I should add some tests though, and I'll put a comment 
there about the connection between query_log_max_queued, number of columns, and 
expression limit.


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

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@428
PS2, Line 428: TQueryTableColumn::TABLES_QUERIED + 1
> Can you make function or macro for this expression?
Mostly indentation, I switched to using continue more to reduce nesting. I 
could make that cleanup a separate patch.

I'll look into adding a macro.


http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@441
PS2, Line 441: "gather_time=" << PrettyPrinter::Print(gather_time, 
TUnit::TIME_NS) << " "
 :   "exec_time=" << PrettyPrinter::Print(exec_time, 
TUnit::TIME_NS);
> Might want to put them as Metric instead.
Metric? I'm not sure I follow.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 24 Apr 2024 23:32:16 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management-flags.cc
File be/src/service/workload-management-flags.cc:

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management-flags.cc@82
PS2, Line 82: DEFINE_int32(query_log_max_queued, 5000
Put a comment to update the default value if TQueryTableColumn change.


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

http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@428
PS2, Line 428: TQueryTableColumn::TABLES_QUERIED + 1
Can you make function or macro for this expression?
The point is to remind people to update the expression if num column change, 
much like what we have for query option:
https://github.com/apache/impala/blob/f620e5d/be/src/service/query-options.h#L55

Is this the only major change in this patch? Others looks like just indentation 
change, new timers, and logging.


http://gerrit.cloudera.org:8080/#/c/21351/2/be/src/service/workload-management.cc@441
PS2, Line 441: "gather_time=" << PrettyPrinter::Print(gather_time, 
TUnit::TIME_NS) << " "
 :   "exec_time=" << PrettyPrinter::Print(exec_time, 
TUnit::TIME_NS);
Might want to put them as Metric instead.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 24 Apr 2024 23:22:06 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

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

Change subject: IMPALA-13012: Lower default query_log_max_queued
..


Patch Set 1:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/16010/ : 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/21351
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Jason Fehr 
Gerrit-Reviewer: Michael Smith 
Gerrit-Reviewer: Riza Suminto 
Gerrit-Comment-Date: Wed, 24 Apr 2024 21:43:28 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-13012: Lower default query log max queued

2024-04-24 Thread Michael Smith (Code Review)
Michael Smith has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/21351


Change subject: IMPALA-13012: Lower default query_log_max_queued
..

IMPALA-13012: Lower default query_log_max_queued

Sets the query_log_max_queued default such that

  query_log_max_queued * num_columns(49) < statement_expression_limit

to avoid triggering e.g.

  AnalysisException: Exceeded the statement expression limit (25)
  Statement has 370039 expressions.

Also increases statement_expression_limit for insertion to avoid an
error if query_log_max_queued is changed.

Logs time taken to write to the queries table for help with debugging.

Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
---
M be/src/service/workload-management-flags.cc
M be/src/service/workload-management.cc
2 files changed, 102 insertions(+), 87 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6535675307d88cb65ba7d908f3c692e0cf3259d7
Gerrit-Change-Number: 21351
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith