Re: Review Request 52493: HIVE-13589 : beeline support prompt for password with '-p' option

[cheng xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52493/#review153648
---


Ship it!




Ship It!

- cheng xu


On Oct. 22, 2016, 1:31 a.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52493/
> ---
> 
> (Updated Oct. 22, 2016, 1:31 a.m.)
> 
> 
> Review request for hive, cheng xu and Sergio Pena.
> 
> 
> Bugs: HIVE-13589
> https://issues.apache.org/jira/browse/HIVE-13589
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-13589 : beeline support prompt for password with '-p' option
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/BeeLine.java 
> fdbe0a2c6c37688ab511710bd43517908996f158 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 6c3e7f70c811ad81aa8f3ee2ec2e42d98da7d330 
>   
> itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52493/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 52493: HIVE-13589 : beeline support prompt for password with '-p' option

[Vihang Karajgaonkar]

> On Oct. 21, 2016, 1:28 a.m., cheng xu wrote:
> > beeline/src/java/org/apache/hive/beeline/BeeLine.java, line 682
> > 
> >
> > hint: perfer HIVE_VAR_PREFIX.equals(arg)

also removed unnecessary brackets which was making it hard to read.


> On Oct. 21, 2016, 1:28 a.m., cheng xu wrote:
> > beeline/src/java/org/apache/hive/beeline/BeeLine.java, line 953
> > 
> >
> > Let's make it simple as following:
> > return url.indexOf('/', "jdbc:hive2://".length() < 0;

also renamed the method and made some changes to improve readability.


- Vihang


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52493/#review153482
---


On Oct. 21, 2016, 5:31 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52493/
> ---
> 
> (Updated Oct. 21, 2016, 5:31 p.m.)
> 
> 
> Review request for hive, cheng xu and Sergio Pena.
> 
> 
> Bugs: HIVE-13589
> https://issues.apache.org/jira/browse/HIVE-13589
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-13589 : beeline support prompt for password with '-p' option
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/BeeLine.java 
> fdbe0a2c6c37688ab511710bd43517908996f158 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 6c3e7f70c811ad81aa8f3ee2ec2e42d98da7d330 
>   
> itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52493/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 52493: HIVE-13589 : beeline support prompt for password with '-p' option

[Vihang Karajgaonkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52493/
---

(Updated Oct. 21, 2016, 5:31 p.m.)


Review request for hive, cheng xu and Sergio Pena.


Changes
---

Addressed review commands


Bugs: HIVE-13589
https://issues.apache.org/jira/browse/HIVE-13589


Repository: hive-git


Description
---

HIVE-13589 : beeline support prompt for password with '-p' option


Diffs (updated)
-

  beeline/src/java/org/apache/hive/beeline/BeeLine.java 
fdbe0a2c6c37688ab511710bd43517908996f158 
  beeline/src/java/org/apache/hive/beeline/Commands.java 
6c3e7f70c811ad81aa8f3ee2ec2e42d98da7d330 
  
itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/52493/diff/


Testing
---


Thanks,

Vihang Karajgaonkar

Re: Review Request 52493: HIVE-13589 : beeline support prompt for password with '-p' option

[cheng xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52493/#review153482
---



Thank you for updating the patch. LGTM overall and just leave some minor 
comments.


beeline/src/java/org/apache/hive/beeline/BeeLine.java (line 680)


hint: perfer HIVE_VAR_PREFIX.equals(arg)



beeline/src/java/org/apache/hive/beeline/BeeLine.java (line 926)


2 space indent



beeline/src/java/org/apache/hive/beeline/BeeLine.java (line 946)


Let's make it simple as following:
return url.indexOf('/', "jdbc:hive2://".length() < 0;



itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
 (line 48)


Please move it before Line 45.



itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
 (line 69)


hint: better to call "testPromptPasswordOptionAsFirst" in consist with the 
later one.



itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
 (line 243)


2 space indent


- cheng xu


On Oct. 21, 2016, 2:02 a.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52493/
> ---
> 
> (Updated Oct. 21, 2016, 2:02 a.m.)
> 
> 
> Review request for hive, cheng xu and Sergio Pena.
> 
> 
> Bugs: HIVE-13589
> https://issues.apache.org/jira/browse/HIVE-13589
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-13589 : beeline support prompt for password with '-p' option
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/BeeLine.java 
> fdbe0a2c6c37688ab511710bd43517908996f158 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 6c3e7f70c811ad81aa8f3ee2ec2e42d98da7d330 
>   
> itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52493/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 52493: HIVE-13589 : beeline support prompt for password with '-p' option

[Vihang Karajgaonkar]

> On Oct. 9, 2016, 1:42 a.m., cheng xu wrote:
> > beeline/src/java/org/apache/hive/beeline/Commands.java, line 1059
> > 
> >
> > This error msg is not user-friendly. Can you give some potential reason 
> > for such NPE like the comments do?

I think we should throw a RuntimeException here since there is nothing that 
Beeline can do here. I have updated the error message and change it to throwing 
RuntimeException. Let me know if you have any thoughts.


> On Oct. 9, 2016, 1:42 a.m., cheng xu wrote:
> > beeline/src/test/org/apache/hive/beeline/TestBeelineArgParsing.java, line 
> > 114
> > 
> >
> > Do we have to change the original format with "key=value;" pair?

Yes, this is required since now the password be optional. The connect(String 
line) method expects connect[driver] where the 
driver value can be present or not. If the password is also optional there is 
no way to distinguish between connectand  
  case. That was the reason I changed the implementation of 
constructCmd method in the first version of the patch.

Now, I have kept the original implementation of the method and added a new 
method which gets called only when -p is used without providing a password. 
This limits the scope of the change and also takes care of both the cases. Let 
me know if you have any thoughts.


> On Oct. 9, 2016, 1:42 a.m., cheng xu wrote:
> > beeline/src/test/resources/hive-site.xml, line 48
> > 
> >
> > Do we need this change?

Yes, this property is used by the testcases to add dummy data in test tables. 
This is similar to what we do in other modules too.


> On Oct. 9, 2016, 1:42 a.m., cheng xu wrote:
> > itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java,
> >  line 59
> > 
> >
> > Why we need this configuration?

removed it.


- Vihang


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52493/#review151931
---


On Oct. 20, 2016, 6:02 p.m., Vihang Karajgaonkar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52493/
> ---
> 
> (Updated Oct. 20, 2016, 6:02 p.m.)
> 
> 
> Review request for hive, cheng xu and Sergio Pena.
> 
> 
> Bugs: HIVE-13589
> https://issues.apache.org/jira/browse/HIVE-13589
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-13589 : beeline support prompt for password with '-p' option
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/BeeLine.java 
> fdbe0a2c6c37688ab511710bd43517908996f158 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 6c3e7f70c811ad81aa8f3ee2ec2e42d98da7d330 
>   
> itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52493/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vihang Karajgaonkar
> 
>

Re: Review Request 52493: HIVE-13589 : beeline support prompt for password with '-p' option

[Vihang Karajgaonkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52493/
---

(Updated Oct. 20, 2016, 6:02 p.m.)


Review request for hive, cheng xu and Sergio Pena.


Changes
---

Added review comments and made minor changes after rebasing to the latest 
changes.


Summary (updated)
-

HIVE-13589 : beeline support prompt for password with '-p' option


Bugs: HIVE-13589
https://issues.apache.org/jira/browse/HIVE-13589


Repository: hive-git


Description (updated)
---

HIVE-13589 : beeline support prompt for password with '-p' option


Diffs (updated)
-

  beeline/src/java/org/apache/hive/beeline/BeeLine.java 
fdbe0a2c6c37688ab511710bd43517908996f158 
  beeline/src/java/org/apache/hive/beeline/Commands.java 
6c3e7f70c811ad81aa8f3ee2ec2e42d98da7d330 
  
itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/52493/diff/


Testing
---


Thanks,

Vihang Karajgaonkar