You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2017/06/25 18:46:33 UTC
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Jim Apple has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7294
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
Reading a uninitialized memory as a boolean is undefined behavior when
it isn't the bit pattern for true or false.
Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
---
M be/src/runtime/disk-io-mgr.h
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/7294/2
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 4:
FWIW in practice it's not possible to re-use a ScanRange object because the lifecycle is a bit messed-up - IMPALA-4249
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7294/2/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS2, Line 584: is_cancelled_ = false;
> Nevermind about my comment of Reset() being used only in test code. It's us
Would you comment on IMPALA-5634 to that effect?
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7294/2/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS2, Line 584: is_cancelled_ = false;
> Done:
Nevermind about my comment of Reset() being used only in test code. It's used here too: https://github.com/apache/incubator-impala/blob/master/be/src/exec/hdfs-scan-node-base.cc#L614
Those bool flags are currently being initialized in InitInternal(). We should consider moving their initialization to Reset() instead.
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 5: Code-Review+2
jenkins failed because:
F0711 18:05:06.155953 23749 tablet_server_main.cc:72] Check failed: _s.ok() Bad status: Service unavailable: Cannot initialize clock: Error reading clock. Clock considered unsynchronized
rebase +2
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 5: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/847/
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
Reading a uninitialized memory as a boolean is undefined behavior when
it isn't the bit pattern for true or false. Clang's undefined behavior
sanitizer caught is_cancelled_ being used before being initialized;
this patch also initializes the other booleans in DiskIoMgr::ScanRange
to prevent similar undefined behavior.
Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Reviewed-on: http://gerrit.cloudera.org:8080/7294
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr.h
2 files changed, 6 insertions(+), 8 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Jim Apple: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#3).
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
Reading a uninitialized memory as a boolean is undefined behavior when
it isn't the bit pattern for true or false. Clang's undefined behavior
sanitizer caught is_cancelled_ being used before being initialized;
this patch also initializes the other booleans in DiskIoMgr::ScanRange
to prevent similar undefined behavior.
Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
---
M be/src/runtime/disk-io-mgr.h
1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/7294/3
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/847/
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7294/2/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS2, Line 584: is_cancelled_ = false;
> I would prefer to keep this patch within the bounds of only fixing the unde
May be I am mistaken but it appears that Reset() is only used in tests. However, if you decide to not do so in this patch, please file a JIRA if necessary so it doesn't get forgotten. It seems that we also need to rest eosr_queued_ and eosr_returned_ and possibly other flags in Reset()
Now that the bool flags are initialized here, the constructor probably doesn't have to do so.
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7294/2/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS2, Line 584: is_cancelled_ = false;
Actually, shouldn't this be done in Reset() instead ?
May also want to fix the constructor to initialize this flag or move the initialization of other bool flags here ?
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has uploaded a new patch set (#4).
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
Reading a uninitialized memory as a boolean is undefined behavior when
it isn't the bit pattern for true or false. Clang's undefined behavior
sanitizer caught is_cancelled_ being used before being initialized;
this patch also initializes the other booleans in DiskIoMgr::ScanRange
to prevent similar undefined behavior.
Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
---
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr.h
2 files changed, 6 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/7294/4
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/849/
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 2:
(1 comment)
Had to rebase to pick up IMPALA-5594, which fixed a broken build with newer Kudu JARs.
http://gerrit.cloudera.org:8080/#/c/7294/2/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS2, Line 584: is_cancelled_ = false;
> Actually, shouldn't this be done in Reset() instead ?
I would prefer to keep this patch within the bounds of only fixing the undefined behavior; I'd suggest that meaningful changes like changing the behavior of Reset() would best fit in a different patch. Thoughts?
In the meantime, I have initialized all other bool flags, as you have suggested.
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Set DiskIoMgr::ScanRange::is cancelled before read
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5031: Set DiskIoMgr::ScanRange::is_cancelled_ before read
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7294/2/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:
PS2, Line 584: is_cancelled_ = false;
> May be I am mistaken but it appears that Reset() is only used in tests. How
Done:
1. Filed IMPALA-5634.
2. Changed constructor as mentioned.
--
To view, visit http://gerrit.cloudera.org:8080/7294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I22898ec96ac44c4902f8072f27453cfc58358cae
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes