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 2019/08/09 00:40:45 UTC

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser 

Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14040


Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................

IMPALA-8850: is_percent not initialized in TmpFileMgr parser

This bug was only detected on ASAN. The is_percent variable
was not initialised and is not set in ParseMemSpec() when it
does an early exit for an empty string. This caused
TmpFileMgr to consider the empty memory spec after the colon
to be invalid.

Testing:
This reliably reproduced in TmpFileMgrTest under ASAN.
Initialising is_percent fixed the issue.

Added more detailed tests for ParseMemSpec() that initialise
is_percent to avoid similar bugs.

Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
---
M be/src/util/parse-util-test.cc
M be/src/util/parse-util.cc
2 files changed, 23 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 1:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Aug 2019 01:20:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 18:13:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 18:14:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

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

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

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

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

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................

IMPALA-8850: is_percent not initialized in TmpFileMgr parser

This bug was only detected on ASAN. The is_percent variable
was not initialised and is not set in ParseMemSpec() when it
does an early exit for an empty string. This caused
TmpFileMgr to consider the empty memory spec after the colon
to be invalid.

Testing:
This reliably reproduced in TmpFileMgrTest under ASAN.
Initialising is_percent fixed the issue.

Added more detailed tests for ParseMemSpec() that initialise
is_percent to avoid similar bugs.

Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
---
M be/src/util/parse-util-test.cc
M be/src/util/parse-util.cc
2 files changed, 22 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 18:14:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Bikramjeet Vig, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................

IMPALA-8850: is_percent not initialized in TmpFileMgr parser

This bug was only detected on ASAN. The is_percent variable
was not initialised and is not set in ParseMemSpec() when it
does an early exit for an empty string. This caused
TmpFileMgr to consider the empty memory spec after the colon
to be invalid.

Testing:
This reliably reproduced in TmpFileMgrTest under ASAN.
Initialising is_percent fixed the issue.

Added more detailed tests for ParseMemSpec() that initialise
is_percent to avoid similar bugs.

Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
---
M be/src/util/parse-util-test.cc
M be/src/util/parse-util.cc
2 files changed, 24 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 4: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 22:21:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 3:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 18:54:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 2:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Aug 2019 01:24:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14040/2/be/src/util/parse-util-test.cc
File be/src/util/parse-util-test.cc:

http://gerrit.cloudera.org:8080/#/c/14040/2/be/src/util/parse-util-test.cc@40
PS2, Line 40:   is_percent = true;
nit: maybe add a short comment here or as a method comment explaining why we are initializing it every time to the opposite of what is expected.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 01:22:41 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14040/2/be/src/util/parse-util-test.cc
File be/src/util/parse-util-test.cc:

http://gerrit.cloudera.org:8080/#/c/14040/2/be/src/util/parse-util-test.cc@40
PS2, Line 40:   is_percent = true;
> nit: maybe add a short comment here or as a method comment explaining why w
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Aug 2019 18:14:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8850: is percent not initialized in TmpFileMgr parser

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/14040 )

Change subject: IMPALA-8850: is_percent not initialized in TmpFileMgr parser
......................................................................

IMPALA-8850: is_percent not initialized in TmpFileMgr parser

This bug was only detected on ASAN. The is_percent variable
was not initialised and is not set in ParseMemSpec() when it
does an early exit for an empty string. This caused
TmpFileMgr to consider the empty memory spec after the colon
to be invalid.

Testing:
This reliably reproduced in TmpFileMgrTest under ASAN.
Initialising is_percent fixed the issue.

Added more detailed tests for ParseMemSpec() that initialise
is_percent to avoid similar bugs.

Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Reviewed-on: http://gerrit.cloudera.org:8080/14040
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/parse-util-test.cc
M be/src/util/parse-util.cc
2 files changed, 24 insertions(+), 1 deletion(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
Gerrit-Change-Number: 14040
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>