You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Lars Volker (Code Review)" <ge...@cloudera.org> on 2018/04/25 19:39:41 UTC
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Lars Volker has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10192
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
IMPALA-6927: Remove invalid DCHECK from coordinator backend state
An invalid DCHECK in Coordinator::BackendState::InstanceStatsToJson()
assumed that the number of fragment instances is equal to the number of
fragments. This is not true when mt_dop > 0, which results in multiple
instances of a fragment on a single node.
The fix is to remove the invalid DCHECK. It served to document the
programmers understanding and the code does not rely on the assumption
being true.
This change adds a test that would fail with the DCHECK in place.
Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
---
M be/src/runtime/coordinator-backend-state.cc
M tests/webserver/test_web_pages.py
2 files changed, 13 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/10192/1
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
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/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
IMPALA-6927: Remove invalid DCHECK from coordinator backend state
An invalid DCHECK in Coordinator::BackendState::InstanceStatsToJson()
assumed that the number of fragment instances is equal to the number of
fragments. This is not true when mt_dop > 0, which results in multiple
instances of a fragment on a single node.
The fix is to remove the invalid DCHECK. It served to document the
programmers understanding and the code does not rely on the assumption
being true.
This change adds a test that would fail with the DCHECK in place.
Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Reviewed-on: http://gerrit.cloudera.org:8080/10192
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/coordinator-backend-state.cc
M tests/webserver/test_web_pages.py
2 files changed, 13 insertions(+), 2 deletions(-)
Approvals:
Sailesh Mukil: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2361/
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Apr 2018 20:24:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc@a619
PS1, Line 619:
This LGTM, but is there any point in having the DCHECK and checking like so:
DCHECK_EQ(instance_stats.Size() * mt_dop, fragments_.size()) ?
If you think it's not worth it, then we can just go ahead with this patch as is.
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Apr 2018 19:48:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc@a619
PS1, Line 619:
> I'm not sure whether that value forces the exact number of instances per fr
Looking at scheduler.cc:440, it seems that there are min(mt_dop, num_scan_ranges) scan nodes.
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Apr 2018 19:54:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc@a619
PS1, Line 619:
> Looking at scheduler.cc:440, it seems that there are min(mt_dop, num_scan_r
Ah ok, that would make this unnecessarily complicated then. Let's leave the patch as it is. Thanks for checking.
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Apr 2018 20:02:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/10192/1/be/src/runtime/coordinator-backend-state.cc@a619
PS1, Line 619:
> This LGTM, but is there any point in having the DCHECK and checking like so
I'm not sure whether that value forces the exact number of instances per fragment or is rather treated as an upper bound. I also wasn't sure whether it would apply to all fragments or only some. Do you know if both are true?
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Apr 2018 19:52:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2361/
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 00:18:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2367/
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 00:26:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6927: Remove invalid DCHECK from coordinator backend state
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10192 )
Change subject: IMPALA-6927: Remove invalid DCHECK from coordinator backend state
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10192
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ea4d583c6ab3fc788db8e220c0a1891918a823f
Gerrit-Change-Number: 10192
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 04:26:49 +0000
Gerrit-HasComments: No