You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Aihua Xu <ax...@cloudera.com> on 2015/11/20 17:56:47 UTC
Review Request 40549: HIVE-12456: QueryId can't be stored in the
configuration of the SessionState since multiple queries can run in a
single session
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40549/
-----------------------------------------------------------
Review request for hive.
Repository: hive-git
Description
-------
HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session
Diffs
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209ad90841740c447534f7902d49c8a8d2038
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 3f2de108f069a815a46d49498238bb8197263d93
service/src/java/org/apache/hive/service/cli/operation/Operation.java d13415e820e67a027d63ba994c05db18b613afc1
service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 8b4226522f1ff6c5c777fe372490f0fd157a3b2b
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 2d784f0d978c04ce674dc06b1adc0cf3d8d5f984
Diff: https://reviews.apache.org/r/40549/diff/
Testing
-------
Thanks,
Aihua Xu
Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the
configuration of the SessionState since multiple queries can run in a
single session
Posted by Aihua Xu <ax...@cloudera.com>.
> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 56
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line56>
> >
> > what does MDC stand for ?
>
> Aihua Xu wrote:
> MDC is log4j term. Stands for Mapped diagnostics context and that is how we used for sessionId and queryId.
>
> Mohit Sabharwal wrote:
> I'd vote to call it QUERYID_LOG_KEY or something to make it more readable.
Good idea. I will update that.
> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 87
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line87>
> >
> > is this conditional needed ?
>
> Aihua Xu wrote:
> Yeah. It's needed. So it means we only overwrite when the passed in confOverlay is not null. Originally I was also confused and thought it's the class memember but actually it's the passed in parameter.
>
> Mohit Sabharwal wrote:
> Ok.
>
> In the current code:
> If param confOverlay == null, this.confOverlay is null (default)
> if param confOverlay != null, this.confOverlay = confOverlay
> So, the conditional is redundant, right ?
this.confOverlay is an empty Map initially. Other places don't check for null but check if it's empty. So we need this to make sure not to set null to it.
- Aihua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40549/#review107399
-----------------------------------------------------------
On Nov. 20, 2015, 4:56 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 4:56 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209ad90841740c447534f7902d49c8a8d2038
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 3f2de108f069a815a46d49498238bb8197263d93
> service/src/java/org/apache/hive/service/cli/operation/Operation.java d13415e820e67a027d63ba994c05db18b613afc1
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 8b4226522f1ff6c5c777fe372490f0fd157a3b2b
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 2d784f0d978c04ce674dc06b1adc0cf3d8d5f984
>
> Diff: https://reviews.apache.org/r/40549/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the
configuration of the SessionState since multiple queries can run in a
single session
Posted by Aihua Xu <ax...@cloudera.com>.
> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 56
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line56>
> >
> > what does MDC stand for ?
MDC is log4j term. Stands for Mapped diagnostics context and that is how we used for sessionId and queryId.
> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, lines 56-57
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line56>
> >
> > why make these public ... only accessed in this class.
Yeah. Originally I marked as private, but in case we need to access such constant somewhere from a client or from a test case. We can directly use that with public access.
> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 87
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line87>
> >
> > is this conditional needed ?
Yeah. It's needed. So it means we only overwrite when the passed in confOverlay is not null. Originally I was also confused and thought it's the class memember but actually it's the passed in parameter.
- Aihua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40549/#review107399
-----------------------------------------------------------
On Nov. 20, 2015, 4:56 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 4:56 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209ad90841740c447534f7902d49c8a8d2038
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 3f2de108f069a815a46d49498238bb8197263d93
> service/src/java/org/apache/hive/service/cli/operation/Operation.java d13415e820e67a027d63ba994c05db18b613afc1
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 8b4226522f1ff6c5c777fe372490f0fd157a3b2b
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 2d784f0d978c04ce674dc06b1adc0cf3d8d5f984
>
> Diff: https://reviews.apache.org/r/40549/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the
configuration of the SessionState since multiple queries can run in a
single session
Posted by Mohit Sabharwal <mo...@cloudera.com>.
> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 56
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line56>
> >
> > what does MDC stand for ?
>
> Aihua Xu wrote:
> MDC is log4j term. Stands for Mapped diagnostics context and that is how we used for sessionId and queryId.
I'd vote to call it QUERYID_LOG_KEY or something to make it more readable.
> On Nov. 20, 2015, 7:30 p.m., Mohit Sabharwal wrote:
> > service/src/java/org/apache/hive/service/cli/operation/Operation.java, line 87
> > <https://reviews.apache.org/r/40549/diff/1/?file=1134612#file1134612line87>
> >
> > is this conditional needed ?
>
> Aihua Xu wrote:
> Yeah. It's needed. So it means we only overwrite when the passed in confOverlay is not null. Originally I was also confused and thought it's the class memember but actually it's the passed in parameter.
Ok.
In the current code:
If param confOverlay == null, this.confOverlay is null (default)
if param confOverlay != null, this.confOverlay = confOverlay
So, the conditional is redundant, right ?
- Mohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40549/#review107399
-----------------------------------------------------------
On Nov. 20, 2015, 4:56 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 4:56 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209ad90841740c447534f7902d49c8a8d2038
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 3f2de108f069a815a46d49498238bb8197263d93
> service/src/java/org/apache/hive/service/cli/operation/Operation.java d13415e820e67a027d63ba994c05db18b613afc1
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 8b4226522f1ff6c5c777fe372490f0fd157a3b2b
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 2d784f0d978c04ce674dc06b1adc0cf3d8d5f984
>
> Diff: https://reviews.apache.org/r/40549/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the
configuration of the SessionState since multiple queries can run in a
single session
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40549/#review107399
-----------------------------------------------------------
LGTM. Couple of questions.
service/src/java/org/apache/hive/service/cli/operation/Operation.java (line 56)
<https://reviews.apache.org/r/40549/#comment166493>
what does MDC stand for ?
service/src/java/org/apache/hive/service/cli/operation/Operation.java (lines 56 - 57)
<https://reviews.apache.org/r/40549/#comment166496>
why make these public ... only accessed in this class.
service/src/java/org/apache/hive/service/cli/operation/Operation.java (line 87)
<https://reviews.apache.org/r/40549/#comment166497>
is this conditional needed ?
- Mohit Sabharwal
On Nov. 20, 2015, 4:56 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 4:56 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209ad90841740c447534f7902d49c8a8d2038
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 3f2de108f069a815a46d49498238bb8197263d93
> service/src/java/org/apache/hive/service/cli/operation/Operation.java d13415e820e67a027d63ba994c05db18b613afc1
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 8b4226522f1ff6c5c777fe372490f0fd157a3b2b
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 2d784f0d978c04ce674dc06b1adc0cf3d8d5f984
>
> Diff: https://reviews.apache.org/r/40549/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the
configuration of the SessionState since multiple queries can run in a
single session
Posted by Mohit Sabharwal <mo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40549/#review107416
-----------------------------------------------------------
Ship it!
Ship It!
- Mohit Sabharwal
On Nov. 20, 2015, 8:39 p.m., Aihua Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40549/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 8:39 p.m.)
>
>
> Review request for hive.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209a
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 3f2de10
> service/src/java/org/apache/hive/service/cli/operation/Operation.java d13415e
> service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 8b42265
> service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 2d784f0
>
> Diff: https://reviews.apache.org/r/40549/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Aihua Xu
>
>
Re: Review Request 40549: HIVE-12456: QueryId can't be stored in the
configuration of the SessionState since multiple queries can run in a
single session
Posted by Aihua Xu <ax...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40549/
-----------------------------------------------------------
(Updated Nov. 20, 2015, 8:39 p.m.)
Review request for hive.
Repository: hive-git
Description
-------
HIVE-12456: QueryId can't be stored in the configuration of the SessionState since multiple queries can run in a single session
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4f8209a
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java 3f2de10
service/src/java/org/apache/hive/service/cli/operation/Operation.java d13415e
service/src/java/org/apache/hive/service/cli/operation/SQLOperation.java 8b42265
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 2d784f0
Diff: https://reviews.apache.org/r/40549/diff/
Testing
-------
Thanks,
Aihua Xu