You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2023/03/15 11:33:27 UTC
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
18770832838@163.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19621
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
The current ImpalaServer::GetQueryRecord() may cause the caller to
access invalid iterators, although the function locks query_log_lock_
in the execution, the query_record it provides cannot guarantee to be
valid, because it is out of the protection of query_log_lock_ after
returning, if query_log_index_ just deletes the corresponding record
at this time, then query_record will be an invalid iterator.
For fix that, this patch uses shared_ptr instead of the original
unique_ptr to extend the lifetime of query_record after GetQueryRecord
returns.
Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
---
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
3 files changed, 69 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/19621/1
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <18...@163.com>
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12628/ : 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/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Mar 2023 11:53:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.h@1207
PS2, Line 1207: is owned by 'query_log_'
nit: Is it still true with changes in this patch?
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Fri, 07 Apr 2023 12:08:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
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/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
The current ImpalaServer::GetQueryRecord() may cause the caller to
access invalid iterators, although the function locks query_log_lock_
in the execution, the query_record it provides cannot guarantee to be
valid, because it is out of the protection of query_log_lock_ after
returning, if the corresponding record in query_log_index_ is just
deleted at this time, then query_record will be an invalid iterator.
To fix that, this patch uses shared_ptr instead of the original
unique_ptr to extend the lifetime of query_record after GetQueryRecord
returns. In addition, a new argument retried_query_record is added to
GetQueryRecord(), the motivation is to acquire query_log_lock_ only once
for getting both the original query record and the retried query record.
Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Reviewed-on: http://gerrit.cloudera.org:8080/19621
Reviewed-by: Quanlong Huang <hu...@gmail.com>
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
3 files changed, 69 insertions(+), 67 deletions(-)
Approvals:
Quanlong Huang: Looks good to me, but someone else must approve
Wenzhe Zhou: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
18770832838@163.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 2:
(3 comments)
Thanks for your review.
http://gerrit.cloudera.org:8080/#/c/19621/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19621/2//COMMIT_MSG@13
PS2, Line 13: if query_log_index_ just deletes the corresponding record
> nit: if the corresponding record in query_log_index_ is just deleted
Done
http://gerrit.cloudera.org:8080/#/c/19621/2//COMMIT_MSG@16
PS2, Line 16: For
> nit: To
Done
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.cc@759
PS2, Line 759: be earlier in the query log
> retried query ran later than the original query, it should be closer to the
Done
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 07:01:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Thanks for fixing this!
http://gerrit.cloudera.org:8080/#/c/19621/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19621/1//COMMIT_MSG@19
PS1, Line 19:
Could you also mention the new argument added in GetQueryRecord()? I think the motivation is to acquire query_log_lock_ only once for getting both the original query record and the retried query record.
http://gerrit.cloudera.org:8080/#/c/19621/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/19621/1/be/src/service/impala-server.h@1209
PS1, Line 1209: std::shared_ptr<QueryStateRecord>*
Continue the discussion of https://gerrit.cloudera.org/c/19417/8/be/src/service/impala-server.h#1226
I think it's tricky to hold raw address of the container item. Some mantanence operations inside the container like reallocation in std::vector might make the addresses invalid. But it seems std::list is safe so I'm ok with this.
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 21 Mar 2023 11:53:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 15:45:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9219/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 05:03:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19621
to look at the new patch set (#2).
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
The current ImpalaServer::GetQueryRecord() may cause the caller to
access invalid iterators, although the function locks query_log_lock_
in the execution, the query_record it provides cannot guarantee to be
valid, because it is out of the protection of query_log_lock_ after
returning, if query_log_index_ just deletes the corresponding record
at this time, then query_record will be an invalid iterator.
For fix that, this patch uses shared_ptr instead of the original
unique_ptr to extend the lifetime of query_record after GetQueryRecord
returns. In addition, a new argument retried_query_record is added to
GetQueryRecord(), the motivation is to acquire query_log_lock_ only once
for getting both the original query record and the retried query record.
Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
---
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
3 files changed, 69 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/19621/2
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
18770832838@163.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.h@1207
PS2, Line 1207: is owned by 'query_log_'
> nit: Is it still true with changes in this patch?
Thanks for your review! The pointer(shared_ptr) of QueryStateRecord is indeed owned by the 'query_log_', so I believe the comment is accurate.
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 02:15:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/12768/ : 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/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 07:20:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9213/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 15:46:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9217/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 21:27:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
18770832838@163.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 2:
Hi Quanlong, thanks for your reply. It looks like there are currently no other reviewers, can we consider merging this? Or are there other tasks need to complete?
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Apr 2023 10:47:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Yifan Zhang, Wenzhe Zhou, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19621
to look at the new patch set (#3).
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
The current ImpalaServer::GetQueryRecord() may cause the caller to
access invalid iterators, although the function locks query_log_lock_
in the execution, the query_record it provides cannot guarantee to be
valid, because it is out of the protection of query_log_lock_ after
returning, if the corresponding record in query_log_index_ is just
deleted at this time, then query_record will be an invalid iterator.
To fix that, this patch uses shared_ptr instead of the original
unique_ptr to extend the lifetime of query_record after GetQueryRecord
returns. In addition, a new argument retried_query_record is added to
GetQueryRecord(), the motivation is to acquire query_log_lock_ only once
for getting both the original query record and the retried query record.
Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
---
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
3 files changed, 69 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/19621/3
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 2:
(3 comments)
Thanks for fixing the issue. Looks good to me.
http://gerrit.cloudera.org:8080/#/c/19621/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19621/2//COMMIT_MSG@13
PS2, Line 13: if query_log_index_ just deletes the corresponding record
nit: if the corresponding record in query_log_index_ is just deleted
http://gerrit.cloudera.org:8080/#/c/19621/2//COMMIT_MSG@16
PS2, Line 16: For
nit: To
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/19621/2/be/src/service/impala-server.cc@759
PS2, Line 759: be earlier in the query log
retried query ran later than the original query, it should be closer to the front of the query_log
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 04:47:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 02:39:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 10:15:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
18770832838@163.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 1:
(2 comments)
Thanks for the code review!
http://gerrit.cloudera.org:8080/#/c/19621/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19621/1//COMMIT_MSG@19
PS1, Line 19:
> Could you also mention the new argument added in GetQueryRecord()? I think
Done
http://gerrit.cloudera.org:8080/#/c/19621/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/19621/1/be/src/service/impala-server.h@1209
PS1, Line 1209: std::shared_ptr<QueryStateRecord>*
> Continue the discussion of https://gerrit.cloudera.org/c/19417/8/be/src/service/impala-server.h#1226
>
> I think it's tricky to hold raw address of the container item. Some
> mantanence operations inside the container like reallocation in
> std::vector might make the addresses invalid. But it seems
> std::list is safe so I'm ok with this.
Your concern is understandable as holding the raw address of the container item can indeed cause issues in some situations. However, we don't have to worry about this with std::list as its member functions guarantee that iterators, references and pointers to elements other than the ones being removed on are always valid. If you think this is not straightforward enough, perhaps we could use QueryLog::const_iterator instead of shared_ptr<QueryStateRecord>*?
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 06:28:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
LGTM. I'll wait to see if there are other reviewers.
http://gerrit.cloudera.org:8080/#/c/19621/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/19621/1/be/src/service/impala-server.h@1209
PS1, Line 1209: std::shared_ptr<QueryStateRecord>*
> > Continue the discussion of https://gerrit.cloudera.org/c/19417/8/be/src/s
The current approach is ok. Just want to link the previous discussion in case other reviewers have the same question.
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 11:31:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 3: -Code-Review
hit IMPALA-11968
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 21:26:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 4: Code-Review+1
> Patch Set 3: -Code-Review
>
> hit IMPALA-11968
rebased this patch to have the fix of IMPALA-12002
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 00:54:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Tue, 11 Apr 2023 05:02:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19621 )
Change subject: IMPALA-11998: Fix potential invalid iterator issue in ImpalaServer::GetQueryRecord()
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9213/
--
To view, visit http://gerrit.cloudera.org:8080/19621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1279558b0d0b2a3ba31a496c0acb84ac9d99f2f1
Gerrit-Change-Number: 19621
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Anonymous Coward <18...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Mon, 10 Apr 2023 20:58:22 +0000
Gerrit-HasComments: No