impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1972/IMPALA-3882: Fix QueryExecState::lock contention
Date Wed, 10 May 2017 23:45:26 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-1972/IMPALA-3882: Fix QueryExecState::lock_ contention
......................................................................


Patch Set 3:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/6707/2//COMMIT_MSG
Commit Message:

PS2, Line 7: yExecState::lock_ contention
> let's not actually solve that problem right now. it doesn't seem necessary 
Done


Line 11: The most common occurrence of this is while loading the webpage of a query
> Yes, but please double check this is safe. From code inspection, I believe 
Ran a stress test and its green (Thanks Mike B.). Also I checked the callers manually and
it looked ok to me too.


Line 18: side effects. Instead,
> As stated above, let's not do that.
Done


Line 22: - As an exception, we don't grab QES::lock_ while the query planning is
> As we discussed, please see if you can add a test that:
Not much success on this front. I'm not able to repro the scenario via tests. I added code
to inject sleep simulating table loading in the exec request and loops that keep polling the
web UI, but I'm not able to get the test failing on the asf-master (without this patch). May
be I'm not generating enough load for lock contention to happen. Thoughts? (I've included
the test in PS2).


http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:

Line 197:   {
> Let's take the QES::lock_ here and not do the promise thing.
Done.


http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

Line 741:     lock_guard<mutex> l(*exec_state->lock());
> same
Done


http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

Line 715:     if (exec_state.get() != nullptr) {
> please fill in those details.
Done


http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS2, Line 814: 
             :   (*exec_state)->query_events()->MarkEvent("Query submitted");
             : 
Changes undone as discussed.


http://gerrit.cloudera.org:8080/#/c/6707/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS3, Line 603:      // For queries in CREATED state, the profile information isn't populated
yet.
             :       if (exec_state->query_state() == beeswax::QueryState::CREATED) return
Status::OK();
Wanted to double check if this change is required. Reason being, with this change, the profile
doesn't show up on the web UI till the metadata is loaded. That might affect some users/clients
relying on the web UI profiles to track the query state?


http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS2, Line 296: 
             :   /// goto http://msdn.microsoft.com/en-us/library/ms714687.aspx
> I think this comment just adds confusion.  It's up to the QueryExecState cl
Done


http://gerrit.cloudera.org:8080/#/c/6707/2/be/src/service/query-exec-state.h
File be/src/service/query-exec-state.h:

PS2, Line 244: 
> we won't need that.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie44daa93e3ae4d04d091261f3ec4891caffe8026
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message