[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


IMPALA-5487: Fix race in RuntimeProfile::toThrift()

node.num_children got initialized before children_ was locked. This
could lead to node.num_children < children_.size(), which made the node
tree in the resulting thrift profiles not deserialize properly.

To fix this, node.num_children needs to be initialized after children_
has been locked.

I tested this by running queries on a private cluster for a while,
making sure that the thrift profiles of running queries could be parsed
correctly.

Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Reviewed-on: http://gerrit.cloudera.org:8080/7154
Reviewed-by: Dan Hecht 
Reviewed-by: Tim Armstrong 
Reviewed-by: Sailesh Mukil 
Tested-by: Impala Public Jenkins
---
M be/src/util/runtime-profile.cc
1 file changed, 1 insertion(+), 3 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Sailesh Mukil: Looks good to me, but someone else must approve
  Tim Armstrong: Looks good to me, but someone else must approve
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/723/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 2: Code-Review+1

LGTM, thanks.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 1:

(1 comment)

Thank you for the comments. I uploaded PS2, where I removed the .reserve().

http://gerrit.cloudera.org:8080/#/c/7154/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS1, Line 764: children_.size()
> Yes, I left it there since it doesn't change correctness and I assumed in t
I removed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 2: Code-Review+2

Please have Tim and Sailesh also finish their reviews.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 1:

> (1 comment)

Let's remove it

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7154/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS1, Line 764: children_.size()
> The reserve() shouldn't matter for correctness, but this is wonky.
Yes, I left it there since it doesn't change correctness and I assumed in the 
average case it will still be usefull. I agree that it is confusing. I could 
either

- Remove it
- Add a comment and explain why it is here
- Move it below where we add the children and where it may matter more

I'm in favor of removing it, the number of children seems to be small in 
general, but if we're concerned about performance we may want to move it down 
where the children are copied.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7154/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS1, Line 764: children_.size()
> What about this?
The reserve() shouldn't matter for correctness, but this is wonky.

Maybe we should just get rid of this line - I doubt avoiding an allocation or 
two on this code makes any measurable difference.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7154/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS1, Line 764: children_.size()
What about this?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5487: Fix race in RuntimeProfile::toThrift()

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

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

Change subject: IMPALA-5487: Fix race in RuntimeProfile::toThrift()
..

IMPALA-5487: Fix race in RuntimeProfile::toThrift()

node.num_children got initialized before children_ was locked. This
could lead to node.num_children < children_.size(), which made the node
tree in the resulting thrift profiles not deserialize properly.

To fix this, node.num_children needs to be initialized after children_
has been locked.

I tested this by running queries on a private cluster for a while,
making sure that the thrift profiles of running queries could be parsed
correctly.

Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
---
M be/src/util/runtime-profile.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7fad4ee2eee1f73e387c1e90a3db265b19b3a0c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker