You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/08/29 22:32:31 UTC
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7885
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
IMPALA-5857: avoid invalid free of hedged read metrics
The libHdfs API documents that the output parameter is unchanged on
error, therefore we do not need to attempt to free it on error.
Testing:
The bug only reproduced under stress. I don't know how to trigger this
error path yet.
Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
---
M be/src/runtime/disk-io-mgr-scan-range.cc
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/7885/1
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc
File be/src/runtime/disk-io-mgr-scan-range.cc:
PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics;
Would it be less confusing if we just initialized 'hedged_metrics' to NULL and left hdfsFreeHedgedReadMetrics() where it was?
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc
File be/src/runtime/disk-io-mgr-scan-range.cc:
PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics;
> Would it be less confusing if we just initialized 'hedged_metrics' to NULL
I think it's coincidental that hdfsFreeHedgedReadMetrics() currently works if the argument is a NULL pointer. It's not documented in the interface. Seems like a bad idea to depend on the behaviour.
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1168/
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
IMPALA-5857: avoid invalid free of hedged read metrics
The libHdfs API documents that the output parameter is unchanged on
error, therefore we do not need to attempt to free it on error.
Testing:
The bug only reproduced under stress. I don't know how to trigger this
error path yet.
Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Reviewed-on: http://gerrit.cloudera.org:8080/7885
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/disk-io-mgr-scan-range.cc
1 file changed, 2 insertions(+), 2 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Matthew Jacobs: Looks good to me, but someone else must approve
Sailesh Mukil: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc
File be/src/runtime/disk-io-mgr-scan-range.cc:
PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics;
> It just calls a free(ptr):
Yeah exactly.
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7885/1/be/src/runtime/disk-io-mgr-scan-range.cc
File be/src/runtime/disk-io-mgr-scan-range.cc:
PS1, Line 354: struct hdfsHedgedReadMetrics* hedged_metrics;
> I think it's coincidental that hdfsFreeHedgedReadMetrics() currently works
It just calls a free(ptr):
http://github.mtv.cloudera.com/CDH/hadoop/blob/9ffaadf806c7a061f3494ebdcaec9d43da24b2bf/hadoop-hdfs-project/hadoop-hdfs/src/main/native/libhdfs/hdfs.c#L174
But I agree that it's better to rely on the documented interface rather than the internal implementation that could change at any point.
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5857: avoid invalid free of hedged read metrics
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5857: avoid invalid free of hedged read metrics
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7885
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I93baf3b672429c0283d7f031ff302aca31e05be4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No