You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org> on 2025/04/04 14:25:07 UTC

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Mihaly Szjatinya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22734

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................

WIP IMPALA-10268: Validate the debug actions when they are set

This patch aims to extract existing verifications on DEBUG_ACTION query
option format onto SetQueryOption() stage, in order to prevent failures
on execution stage. Also it localizes verification code for two existing
types of debug actions.

There are two types of debug actions, global e.g. 'RECVR_ADD_BATCH:FAIL'
and ExecNode debug actions, e.g. '0:GETNEXT:FAIL'. Two types are
implemented independetely in source code, both having verification code
intertwined with execution.

In case of global debug actions most of the code inside exising
DebugActionImpl() consists of verification, therefore it make sense to
make a wrapper around it for separating out the execution code.

Things are worse for ExecNode debug actions, where verification code is
sparsed between DebugOptions() constructor and
ExecNode::ExecDebugActionImpl(). Additionaly some verification in
constructor produces warnings, while ExecDebugActionImpl() verification
either fails on DCHECK() or (in single case) returns error. For this
case reasonable solution seems to be simply calling the constructor for
temporary object and extracting verification code from
ExecDebugActionImpl(). This has the drawback of having the same warning
being produced two times.

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

Note: In the long run it is better to write a single verification
routine for both Global and ExecNode debug actions, ideally as part of a
general unification of the two existing debug_action mechanisms. With
this in mind, current patch intends to preserve current behaviour, while
avoiding complex refactoring.

Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/runtime/debug-options.cc
M be/src/runtime/debug-options.h
M be/src/service/query-options.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
A tests/query_test/test_debug_action.py
8 files changed, 198 insertions(+), 36 deletions(-)

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 1
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

This patch aims to extract existing verifications on DEBUG_ACTION query
option format onto pre-planner stage SetQueryOption(), in order to
prevent failures on execution stage. Also, it localizes verification
code for two existing types of debug actions.

There are two types of debug actions, global e.g. 'RECVR_ADD_BATCH:FAIL'
and ExecNode debug actions, e.g. '0:GETNEXT:FAIL'. Two types are
implemented independently in source code, both having verification code
intertwined with execution. In addition, global debug actions subdivide
into C++ and Java, the two being more or less synchronized though.

In case of global debug actions, most of the code inside existing
DebugActionImpl() consists of verification, therefore it makes sense to
make a wrapper around it for separating out the execution code.

Things are worse for ExecNode debug actions, where verification code
consists of two parts, one in DebugOptions() constructor and another one
in ExecNode::ExecDebugActionImpl(). Additionally, some verification in
constructor produces warnings, while ExecDebugActionImpl() verification
either fails on DCHECK() or (in a single case) returns an error. For
this case, a reasonable solution seems to be simply calling the
constructor for a temporary object and extracting verification code from
ExecNode::ExecDebugActionImpl(). This has the drawback of having the
same warning being produced two times.

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

Note: In the long run, it is better to write a single verification
routine for both Global and ExecNode debug actions, ideally as part of a
general unification of the two existing debug_action mechanisms. With
this in mind, the current patch intends to preserve current behavior,
while avoiding complex refactoring.

Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Reviewed-on: http://gerrit.cloudera.org:8080/22734
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/runtime/debug-options.cc
M be/src/runtime/debug-options.h
M be/src/service/query-options.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
A tests/query_test/test_debug_action.py
8 files changed, 204 insertions(+), 45 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 12
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 6: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py
File tests/query_test/test_debug_action.py:

http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py@84
PS6, Line 84:   @classmethod
            :   def get_workload(cls):
            :     return 'functional-query'
nit: This can be removed.


http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py@95
PS6, Line 95:     # Debug actions should be independent of file format, so only testing for
            :     # table_format=parquet/none in order to avoid a test dimension explosion.
            :     cls.ImpalaTestMatrix.add_constraint(lambda v:
            :         v.get_value('table_format').file_format == 'parquet'
            :         and v.get_value('table_format').compression_codec == 'none')
You can replace this with a fixed table_format dimension.

    cls.ImpalaTestMatrix.add_dimension(create_parquet_dimension())



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 6
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Apr 2025 15:05:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 10: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 10
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 16:19:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 3:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 3
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sun, 06 Apr 2025 14:14:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

Things are worse for ExecNode debug actions, where verification code is
sparsed between DebugOptions() constructor and
ExecNode::ExecDebugActionImpl(). Additionally, some verification in
constructor produces warnings, while ExecDebugActionImpl() verification
either fails on DCHECK() or (in a single case) returns an error. For
this case, a reasonable solution seems to be simply calling the
constructor for a temporary object and extracting verification code from
ExecNode::ExecDebugActionImpl(). This has the drawback of having the
same warning being produced two times.

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
---
A __ign
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/runtime/debug-options.cc
M be/src/runtime/debug-options.h
M be/src/service/query-options.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
A tests/query_test/test_debug_action.py
9 files changed, 211 insertions(+), 45 deletions(-)

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 5
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 5:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 5
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Apr 2025 10:22:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 2:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 2
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Apr 2025 15:02:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@319
PS4, Line 319: "invalid command"
> There are many other error types in DebugActionImpl used just like this, so
I mean just the string "invalid command", not the whole Status.
Maybe even change it to more something specific like "invalid DEBUG_ACTION option".



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Apr 2025 14:29:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 10
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/5/__ign
File __ign:

http://gerrit.cloudera.org:8080/#/c/22734/5/__ign@1
PS5, Line 1: 
> Is this a mistake check in?
Oh yeah, sorry



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 6
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Apr 2025 14:56:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/22734/5/__ign
File __ign:

http://gerrit.cloudera.org:8080/#/c/22734/5/__ign@1
PS5, Line 1: Subproject commit 4ee0b574bf2a0f44c30a38b919cc04a95630cb5c
Is this a mistake check in?


http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@235
PS4, Line 235: VerifyExecNodeDeb
> Good idea, ack.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 5
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Apr 2025 14:46:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/22734/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/22734/8//COMMIT_MSG@25
PS8, Line 25: consist
> nit. Not sure what "sparsed" means here, using a simpler word might help ma
Ack


http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/exec/exec-node.cc@476
PS8, Line 476:   if (!param.empty()) {
> Maybe better adding DCHECK(sleep_duration_ms != nullptr)
Yes, why not.


http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/service/query-options.cc@316
PS8, Line 316:         // Verify General DebugAction's first. In case it's valid proceed to
             :         // ExecNode verification. 'invali
> Maybe clarify this comment a bit. Like, "Verify General DebugAction's first
Ack, hope made it more clear.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 10
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 10:22:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@319
PS4, Line 319: "invalid command"
> I mean just the string "invalid command", not the whole Status.
I got that, in DebugActionImpl() there's already a constant string ERROR_MSG for the entire status and 'invalid command' is just part of it, along with many other options like, 'invalid probability', etc, all which are used simply as literal. So perhaps as much as it would add some convenience, it also would introduce some inconsistency. But it's something really minor, I'll add it if you think it's better.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Apr 2025 17:52:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 10: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 10
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 16:19:59 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc@459
PS4, Line 459:       if (!verify_only) {
             :         DCHECK(false) << "Invalid debug action";
             :       }
             :       return Status(Substitute(ERROR_MSG, components[0], action_str, "invalid command"));
> Is it possible to move the ExecNode DebugAction verification here?
Sorry, should be like this.

      if (!verify_only) {
        DCHECK(false) << "Invalid debug action";
        return Status(Substitute(ERROR_MSG, components[0], action_str, "invalid command"));
      } else {
        // An invalid General DebugAction might be a valid ExecNode DebugAction.
        // In verification mode, parse it to TDebugOptions and verify using
        // VerifyExecNodeDebugAction.
        DebugOptions debug_options(value);
        RETURN_IF_ERROR(VerifyExecNodeDebugAction(debug_options.ToThrift()));
      }



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Apr 2025 15:00:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@235
PS4, Line 235: VerifyExecNodeDeb
> Rename to VerifyExecNodeDebugAction.
Good idea, ack.


http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc@459
PS4, Line 459:       if (!verify_only) {
             :         DCHECK(false) << "Invalid debug action";
             :       }
             :       return Status(Substitute(ERROR_MSG, components[0], action_str, "invalid command"));
> Sorry, should be like this.
This can be done, but it would require some cross referencing: query-options.cc::SetQueryOption() -> debug-util.cc::DebugActionImpl() -> query-options.cc::VerifyExecNodeDebugAction(). But more generally, I'd really like to keep General and ExecNode debug actions independent, as they are in the code, and not call one from another.

Absolutely General and ExecNode parsing can be implemented better together, handling all corner cases, but as I describe in commit message, imho that work would better be done in scope of more general refactoring to avoid kind of half-way solution.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 5
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Apr 2025 09:55:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@319
PS4, Line 319: "invalid command"
> I see. You are right.
Yes all errors should be propagated, except this one special case: https://github.com/mszjat/impala/blob/test_debugaction_wrapper/be/src/util/debug-util.cc#L462

Because this can be from trying interpreting ExecNode debug action, so in this case we give it a try with further 'VerifyDebugAction()'



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Apr 2025 10:05:01 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 7
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 8:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 8
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Apr 2025 12:41:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 7:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 7
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Apr 2025 12:36:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 10: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 10
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 16:00:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Apr 2025 13:50:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@319
PS4, Line 319: "invalid command"
> Consider making this a constant string and use it both here and in DebugAct
There are many other error types in DebugActionImpl used just like this, so probably not worth it to make constant just for this case.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Apr 2025 14:26:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.h@353
PS3, Line 353:   MemPool* expr_perm_pool() { return expr_perm_pool_.get(); }
> Move this to query-options.h?
Agree, moved.


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc@466
PS3, Line 466:     RETURN_IF_ERROR(
             :         ParseAndValidateSleepDuration(debug_options_.action_param, &sleep_duration_ms));
             :     VLOG(1) << "DEBUG_ACTION: Sleeping for " << sleep_duration_ms << "ms";
             :     S
> This can be removed if ExecNode::VerifyDebugAction() is called right after 
I moved 'VerifyDebugAction()' to query-options.h  as proposed in another thread, and refactored it a bit. So we probably don't want to have cross references from query-options.h here and back.


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307
PS3, Line 307: break;
             :       }
             :       case TImpalaQueryOptions::NUM_SCANNER_THREADS: {
             :         i
> Why not simply this?
'value' string may contain both General and ExecNode debug actions. 'DebugActionVerifyOnly' checks for General ones and in case of error status we return. But if the error is just 'invalid command' it's possible that there's an ExecNode which needs to be verified below in 'ExecNode::VerifyDebugAction'. 

It's not a perfect logic still, but perhaps the best we can do given independent / different implementations of two debug option types.


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307
PS3, Line 307:         break;
             :       }
             :       case TImpalaQueryOptions::NUM_SCANNER_THREADS: {
             :         int32_t int32_t_val = 0;
             :         RETURN_IF_ERROR(QueryOptionParser::Parse<int32_t>(option, value, &int32_t_val));
             :         query_options->__set_num_scanner_threads(int32_t_val);
> Please leave a short comment on why validation need to go through 2 differe
Done


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h
File be/src/util/debug-util.h:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h@147
PS3, Line 147: /// Slow path implementing DebugAction() for the case where 'debug_action' is non-empty.
> Please document what is the impact of verify_only value.
Ack


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.cc@366
PS3, Line 366:     if (!verify_only) {
> nit: I think this will be better understood if the condition is flipped.
Hmm, on the other hand this way it's more consistent with the checks below, all of which are 'if (!verify_only)'. If you don't insist I would keep it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Apr 2025 13:23:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@235
PS4, Line 235: VerifyDebugAction
Rename to VerifyExecNodeDebugAction.


http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@319
PS4, Line 319: "invalid command"
> Yes all errors should be propagated, except this one special case: https://
Thanks for your explanation.

I understand know that, the reason this specific "invalid command" catched here is because there is a possibility that this is a valid ExecNode DebugAction, even if it is an invalid General DebugAction.


http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc@459
PS4, Line 459:       if (!verify_only) {
             :         DCHECK(false) << "Invalid debug action";
             :       }
             :       return Status(Substitute(ERROR_MSG, components[0], action_str, "invalid command"));
Is it possible to move the ExecNode DebugAction verification here?

      bool throwStatus = true;
      if (!verify_only) {
        DCHECK(false) << "Invalid debug action";
        return Status(Substitute(ERROR_MSG, components[0], action_str, "invalid command"));
      } else {
        // An invalid General DebugAction might be a valid ExecNode DebugAction.
        // In verification mode, parse it to TDebugOptions and verify using
        // VerifyExecNodeDebugAction.
        DebugOptions debug_options(value);
        RETURN_IF_ERROR(VerifyExecNodeDebugAction(debug_options.ToThrift()));
        Status verificationStatus = VerifyExecNodeDebugAction(debug_options.ToThrift());
      }

Thus, DebugActionVerifyOnly will be consistent. It always validates both possibility of General and ExecNode DebugAction.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Apr 2025 14:58:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 8
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................

WIP IMPALA-10268: Validate the debug actions when they are set

In case of global debug actions most of the code inside exising
DebugActionImpl() consists of verification, therefore it make sense to
make a wrapper around it for separating out the execution code.

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

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

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................

WIP IMPALA-10268: Validate the debug actions when they are set

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

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

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 11: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 11
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 16:20:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 6:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 6
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Apr 2025 15:21:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 7:

(1 comment)

gerrit-auto-critic failed. You can reproduce it locally using command:

  python3 bin/jenkins/critique-gerrit-review.py --dryrun

To run it, you might need a virtual env with Python3's venv installed.

http://gerrit.cloudera.org:8080/#/c/22734/7/tests/query_test/test_debug_action.py
File tests/query_test/test_debug_action.py:

http://gerrit.cloudera.org:8080/#/c/22734/7/tests/query_test/test_debug_action.py@24
PS7, Line 24: class TestDebugAction(ImpalaTestSuite):
flake8: E302 expected 2 blank lines, found 1



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 7
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Apr 2025 12:10:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 8: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py
File tests/query_test/test_debug_action.py:

http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py@95
PS6, Line 95:     result = self.execute_query_expect_failure(
            :         self.client, self._query, vector.get_value('exec_option'))
            :     assert self._debug_actions[vector.get_value('debug_action')] in str(result)
            : 
            : 
> Ack
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 8
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Apr 2025 16:05:24 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Hello Riza Suminto, Yida Wu, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

This patch aims to extract existing verifications on DEBUG_ACTION query
option format onto pre-planner stage SetQueryOption(), in order to
prevent failures on execution stage. Also, it localizes verification
code for two existing types of debug actions.

There are two types of debug actions, global e.g. 'RECVR_ADD_BATCH:FAIL'
and ExecNode debug actions, e.g. '0:GETNEXT:FAIL'. Two types are
implemented independently in source code, both having verification code
intertwined with execution. In addition, global debug actions subdivide
into C++ and Java, the two being more or less synchronized though.

In case of global debug actions, most of the code inside existing
DebugActionImpl() consists of verification, therefore it makes sense to
make a wrapper around it for separating out the execution code.

Things are worse for ExecNode debug actions, where verification code
consists of two parts, one in DebugOptions() constructor and another one
in ExecNode::ExecDebugActionImpl(). Additionally, some verification in
constructor produces warnings, while ExecDebugActionImpl() verification
either fails on DCHECK() or (in a single case) returns an error. For
this case, a reasonable solution seems to be simply calling the
constructor for a temporary object and extracting verification code from
ExecNode::ExecDebugActionImpl(). This has the drawback of having the
same warning being produced two times.

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

Note: In the long run, it is better to write a single verification
routine for both Global and ExecNode debug actions, ideally as part of a
general unification of the two existing debug_action mechanisms. With
this in mind, the current patch intends to preserve current behavior,
while avoiding complex refactoring.

Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
---
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/runtime/debug-options.cc
M be/src/runtime/debug-options.h
M be/src/service/query-options.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
A tests/query_test/test_debug_action.py
8 files changed, 202 insertions(+), 45 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 9
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@319
PS4, Line 319: "invalid command"
> I got that, in DebugActionImpl() there's already a constant string ERROR_MS
I see. You are right.
But in that case, isn't it safe to return all non-OK status from DebugActionVerifyOnly()?

  RETURN_IF_ERROR(DebugActionVerifyOnly(value));

If verification pass, then there should be no non-OK status returned.
The only non-OK status that is not using ERROR_MSG template is Status(TErrorCode::INTERNAL_ERROR, error_msg), and it is only possible to throw if verify_only is False.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Apr 2025 19:34:29 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.h@353
PS3, Line 353:   static Status VerifyDebugAction(const TDebugOptions& debug_options);
Move this to query-options.h?


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc@466
PS3, Line 466:     if (!debug_options_.action_param.empty()) {
             :       RETURN_IF_ERROR(
             :           ParseAndValidateSleepDuration(debug_options_.action_param, &sleep_duration_ms));
             :     }
This can be removed if ExecNode::VerifyDebugAction() is called right after L438.
It also make sense to do so, in case VerifyDebugAction is expanded to validate other things later.


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307
PS3, Line 307: Status status = DebugActionVerifyOnly(value);
             :         if (!status.ok() && string::npos == status.msg().msg().find("invalid command")){
             :           return status;
             :         }
Why not simply this?

  RETURN_IF_ERROR(DebugActionVerifyOnly(value));


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307
PS3, Line 307:         Status status = DebugActionVerifyOnly(value);
             :         if (!status.ok() && string::npos == status.msg().msg().find("invalid command")){
             :           return status;
             :         }
             :         DebugOptions debug_options(value);
             :         RETURN_IF_ERROR(ExecNode::VerifyDebugAction(debug_options.ToThrift()));
Please leave a short comment on why validation need to go through 2 different functions: DebugActionVerifyOnly and VerifyDebugAction.


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h
File be/src/util/debug-util.h:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h@147
PS3, Line 147: /// Slow path implementing DebugAction() for the case where 'debug_action' is non-empty.
Please document what is the impact of verify_only value.


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.cc
File be/src/util/debug-util.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.cc@366
PS3, Line 366:     if (!verify_only) {
nit: I think this will be better understood if the condition is flipped.

  if (verify_only) {
    // Only validate the action_str going forward.
    ...
  } else {
    ...
  }



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 3
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Apr 2025 19:24:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 1
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Apr 2025 14:52:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py
File tests/query_test/test_debug_action.py:

http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py@84
PS6, Line 84:   @classmethod
            :   def add_test_dimensions(cls):
            :     super(TestDebugAction, cl
> nit: This can be removed.
Ack


http://gerrit.cloudera.org:8080/#/c/22734/6/tests/query_test/test_debug_action.py@95
PS6, Line 95:         self.client, self._query, vector.get_value('exec_option'))
            :     assert self._debug_actions[vector.get_value('debug_action')] in str(result)
            : 
            : 
            : 
> You can replace this with a fixed table_format dimension.
Ack



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 7
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Apr 2025 12:09:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 7:

Thank you


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 7
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Apr 2025 12:09:21 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Mihaly Szjatinya (Code Review)" <ge...@cloudera.org>.

Mihaly Szjatinya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................

IMPALA-10268: Validate the debug actions when they are set

Finally, having extracted verification code for both types, logic in
impala::SetQueryOption() combines the two verification mechanisms.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 6
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.

Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 8: Code-Review+1

(4 comments)

LGTM, some minor comments

http://gerrit.cloudera.org:8080/#/c/22734/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/22734/8//COMMIT_MSG@25
PS8, Line 25: sparsed
nit. Not sure what "sparsed" means here, using a simpler word might help make it clearer


http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/exec/exec-node.cc@476
PS8, Line 476:   if (!param.empty()) {
Maybe better adding DCHECK(sleep_duration_ms != nullptr)


http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/service/query-options.cc@316
PS8, Line 316:         // Verify General DebugAction's first. In case it passes or 'invalid command'
             :         // verify ExecNode DebugAction's.
Maybe clarify this comment a bit. Like, "Verify General DebugAction's first. If it's valid or just an 'invalid command', continue with ExecNode validation."


http://gerrit.cloudera.org:8080/#/c/22734/8/be/src/service/query-options.cc@316
PS8, Line 316:         // Verify General DebugAction's first. In case it passes or 'invalid command'
             :         // verify ExecNode DebugAction's.
'invalid command' might indicate an ExecNode DebugAction? Might be worth adding a brief comment to explain the rationale, if this is expected behavior



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 8
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 23 Apr 2025 23:04:36 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] WIP IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: WIP IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 1:

(28 comments)

gerrit-auto-critic failed. You can reproduce it locally using command:

  python3 bin/jenkins/critique-gerrit-review.py --dryrun

To run it, you might need a virtual env with Python3's venv installed.

http://gerrit.cloudera.org:8080/#/c/22734/1/be/src/runtime/debug-options.cc
File be/src/runtime/debug-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/1/be/src/runtime/debug-options.cc@40
PS1, Line 40:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py
File tests/query_test/test_debug_action.py:

http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@31
PS1, Line 31: '
flake8: E131 continuation line unaligned for hanging indent


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@38
PS1, Line 38:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@38
PS1, Line 38:     'RECVR_ADD_BATCH:JITTER@8@8': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@40
PS1, Line 40:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@40
PS1, Line 40:     'RECVR_ADD_BATCH:FAIL@8': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@42
PS1, Line 42:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@42
PS1, Line 42:     'RECVR_ADD_BATCH:EXCEPTION@Unknown': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@46
PS1, Line 46:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@46
PS1, Line 46:     '0:GETNEXT:DELAY@aa': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@50
PS1, Line 50:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@50
PS1, Line 50:     'RECVR_ADD_BATCH:FAIL@0.8|0:GETNEXT:FAIL': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@54
PS1, Line 54:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@54
PS1, Line 54:     '0:GETNEXT:FAIL|RECVR_ADD_BATCH:FAIL@0.8': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@58
PS1, Line 58:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@58
PS1, Line 58:     'RECVR_ADD_BATCH:FAIL@8|0:GETNEXT:FAIL': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@62
PS1, Line 62:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@62
PS1, Line 62:     '0:GETNEXT:FAIL|RECVR_ADD_BATCH:FAIL@8': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@66
PS1, Line 66:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@66
PS1, Line 66:     'RECVR_ADD_BATCH:FAIL@0.8|0:GETNEXT:DELAY@aa': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@70
PS1, Line 70:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@70
PS1, Line 70:     '0:GETNEXT:DELAY@aa|RECVR_ADD_BATCH:FAIL@0.8': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@72
PS1, Line 72:   
flake8: W293 blank line contains whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@72
PS1, Line 72:   
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@74
PS1, Line 74:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@74
PS1, Line 74:     'RECVR_ADD_BATCH:FAIL@8|0:GETNEXT:DELAY@aa': 
line has trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@78
PS1, Line 78:  
flake8: W291 trailing whitespace


http://gerrit.cloudera.org:8080/#/c/22734/1/tests/query_test/test_debug_action.py@78
PS1, Line 78:     '0:GETNEXT:DELAY@aa|RECVR_ADD_BATCH:FAIL@8': 
line has trailing whitespace



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 1
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Apr 2025 14:27:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 10:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 10
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 10:50:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 11:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 11
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 16:20:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 11: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 11
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 25 Apr 2025 22:25:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10268: Validate the debug actions when they are set

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )

Change subject: IMPALA-10268: Validate the debug actions when they are set
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc@466
PS3, Line 466:     RETURN_IF_ERROR(
             :         ParseAndValidateSleepDuration(debug_options_.action_param, &sleep_duration_ms));
             :     VLOG(1) << "DEBUG_ACTION: Sleeping for " << sleep_duration_ms << "ms";
             :     S
> I moved 'VerifyDebugAction()' to query-options.h  as proposed in another th
Done


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307
PS3, Line 307: break;
             :       }
             :       case TImpalaQueryOptions::NUM_SCANNER_THREADS: {
             :         i
> 'value' string may contain both General and ExecNode debug actions. 'DebugA
Done


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307
PS3, Line 307:         break;
             :       }
             :       case TImpalaQueryOptions::NUM_SCANNER_THREADS: {
             :         int32_t int32_t_val = 0;
             :         RETURN_IF_ERROR(QueryOptionParser::Parse<int32_t>(option, value, &int32_t_val));
             :         query_options->__set_num_scanner_threads(int32_t_val);
> Done
Done


http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@319
PS4, Line 319: "invalid command"
Consider making this a constant string and use it both here and in DebugActionImpl.


http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h
File be/src/util/debug-util.h:

http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h@147
PS3, Line 147: /// Slow path implementing DebugAction() for the case where 'debug_action' is non-empty.
> Ack
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5
Gerrit-Change-Number: 22734
Gerrit-PatchSet: 4
Gerrit-Owner: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Mihaly Szjatinya <ms...@pm.me>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Apr 2025 14:12:22 +0000
Gerrit-HasComments: Yes