Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-13 Thread Robert Nettleton 


> On May 12, 2016, 1:35 p.m., Robert Nettleton wrote:
> > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/view/VList.java,
> >  line 46
> > 
> >
> > This change is problematic, since the Ambari Integration API already 
> > relies on the return results from the LogSearch Server.
> > 
> > At this point in the dev cycle, we can't be changing the API at all 
> > from LogSearch, or the Ambari Integration will be broken.
> > 
> > Because this is one of the types serialized in responses to the REST 
> > API, this is a part of the API, and at this point in the dev cycle should 
> > be frozen.  
> > 
> > I'd request that this part of the patch be reverted. 
> > 
> > Thanks.
> 
> Dharmesh Makwana wrote:
> I have reverted the resultProperty changes, thanks.

Great.  Thanks for fixing this.


- Robert


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


On May 13, 2016, 6:48 a.m., Dharmesh Makwana wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47296/
> ---
> 
> (Updated May 13, 2016, 6:48 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
> Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
> Sebastian Toader.
> 
> 
> Bugs: AMBARI-16629
> https://issues.apache.org/jira/browse/AMBARI-16629
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Patch contains
> 1) Authentication : change the API as per suggestion and support for roles in 
> authentication process.
> 2) resultSize property removed from collection class.
> 3) "Preview" issue fixed.
> 
> 
> Diffs
> -
> 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
>  0442cf9 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
>  a60402e 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
>  cc61127 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
>  8535039 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
>  79a414c 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
>  315d736 
> 
> Diff: https://reviews.apache.org/r/47296/diff/
> 
> 
> Testing
> ---
> 
> Tested in my local Ambari environment.
> 
> 
> Thanks,
> 
> Dharmesh Makwana
> 
>

Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-13 Thread Robert Nettleton 

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


Ship it!




Ship It!

- Robert Nettleton


On May 13, 2016, 6:48 a.m., Dharmesh Makwana wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47296/
> ---
> 
> (Updated May 13, 2016, 6:48 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
> Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
> Sebastian Toader.
> 
> 
> Bugs: AMBARI-16629
> https://issues.apache.org/jira/browse/AMBARI-16629
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Patch contains
> 1) Authentication : change the API as per suggestion and support for roles in 
> authentication process.
> 2) resultSize property removed from collection class.
> 3) "Preview" issue fixed.
> 
> 
> Diffs
> -
> 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
>  0442cf9 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
>  a60402e 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
>  cc61127 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
>  8535039 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
>  79a414c 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
>  315d736 
> 
> Diff: https://reviews.apache.org/r/47296/diff/
> 
> 
> Testing
> ---
> 
> Tested in my local Ambari environment.
> 
> 
> Thanks,
> 
> Dharmesh Makwana
> 
>

Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-13 Thread Dharmesh Makwana 


> On May 12, 2016, 1:35 p.m., Robert Nettleton wrote:
> > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java,
> >  line 56
> > 
> >
> > Wouldn't it be simpler to just override the toString() methods for this 
> > enumerated type?

Sure, made the change.


- Dharmesh


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


On May 13, 2016, 6:48 a.m., Dharmesh Makwana wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47296/
> ---
> 
> (Updated May 13, 2016, 6:48 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
> Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
> Sebastian Toader.
> 
> 
> Bugs: AMBARI-16629
> https://issues.apache.org/jira/browse/AMBARI-16629
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Patch contains
> 1) Authentication : change the API as per suggestion and support for roles in 
> authentication process.
> 2) resultSize property removed from collection class.
> 3) "Preview" issue fixed.
> 
> 
> Diffs
> -
> 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
>  0442cf9 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
>  a60402e 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
>  cc61127 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
>  8535039 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
>  79a414c 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
>  315d736 
> 
> Diff: https://reviews.apache.org/r/47296/diff/
> 
> 
> Testing
> ---
> 
> Tested in my local Ambari environment.
> 
> 
> Thanks,
> 
> Dharmesh Makwana
> 
>

Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-13 Thread Dharmesh Makwana 


> On May 12, 2016, 3:37 p.m., Sumit Mohanty wrote:
> > ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/view/VList.java,
> >  line 147
> > 
> >
> > Any reason this is being removed?

We were not using that property anymore, for now keeping that property as it is.


- Dharmesh


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


On May 13, 2016, 6:48 a.m., Dharmesh Makwana wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47296/
> ---
> 
> (Updated May 13, 2016, 6:48 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
> Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
> Sebastian Toader.
> 
> 
> Bugs: AMBARI-16629
> https://issues.apache.org/jira/browse/AMBARI-16629
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Patch contains
> 1) Authentication : change the API as per suggestion and support for roles in 
> authentication process.
> 2) resultSize property removed from collection class.
> 3) "Preview" issue fixed.
> 
> 
> Diffs
> -
> 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
>  0442cf9 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
>  a60402e 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
>  cc61127 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
>  8535039 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
>  79a414c 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
>  315d736 
> 
> Diff: https://reviews.apache.org/r/47296/diff/
> 
> 
> Testing
> ---
> 
> Tested in my local Ambari environment.
> 
> 
> Thanks,
> 
> Dharmesh Makwana
> 
>

Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-13 Thread Dharmesh Makwana 

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

(Updated May 13, 2016, 6:48 a.m.)


Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
Sebastian Toader.


Changes
---

Reverted the resultSize property and review comments changes made.


Bugs: AMBARI-16629
https://issues.apache.org/jira/browse/AMBARI-16629


Repository: ambari


Description
---

Patch contains
1) Authentication : change the API as per suggestion and support for roles in 
authentication process.
2) resultSize property removed from collection class.
3) "Preview" issue fixed.


Diffs (updated)
-

  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
 0442cf9 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
 a60402e 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
 cc61127 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
 8535039 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
 79a414c 
  
ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
 315d736 

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


Testing
---

Tested in my local Ambari environment.


Thanks,

Dharmesh Makwana

Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-12 Thread Sumit Mohanty 

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




ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/view/VList.java
 


Any reason this is being removed?


- Sumit Mohanty


On May 12, 2016, 11:31 a.m., Dharmesh Makwana wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47296/
> ---
> 
> (Updated May 12, 2016, 11:31 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
> Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
> Sebastian Toader.
> 
> 
> Bugs: AMBARI-16629
> https://issues.apache.org/jira/browse/AMBARI-16629
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Patch contains
> 1) Authentication : change the API as per suggestion and support for roles in 
> authentication process.
> 2) resultSize property removed from collection class.
> 3) "Preview" issue fixed.
> 
> 
> Diffs
> -
> 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
>  0442cf9 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
>  a60402e 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
>  cc61127 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
>  8535039 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/view/VList.java
>  97226d2 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
>  79a414c 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
>  315d736 
> 
> Diff: https://reviews.apache.org/r/47296/diff/
> 
> 
> Testing
> ---
> 
> Tested in my local Ambari environment.
> 
> 
> Thanks,
> 
> Dharmesh Makwana
> 
>

Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-12 Thread Robert Nettleton 

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



Thanks for contributing this patch.

I believe that some more work needs to be done here before this patch gets 
approved.

In particular, please see my comments on the VList.java changes.  I believe 
this change will break the existing REST API usage in the Ambari Server 
integration layer, and so I'd recommend that at least that portion of the patch 
be reverted.

Thanks.


ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/view/VList.java
 


This change is problematic, since the Ambari Integration API already relies 
on the return results from the LogSearch Server.

At this point in the dev cycle, we can't be changing the API at all from 
LogSearch, or the Ambari Integration will be broken.

Because this is one of the types serialized in responses to the REST API, 
this is a part of the API, and at this point in the dev cycle should be frozen. 
 

I'd request that this part of the patch be reverted. 

Thanks.



ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
 (line 53)


Looks like a type here, can you please fix this enum type to look like:

"PRIVILEGE_INFO"

Thanks



ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
 (line 56)


Wouldn't it be simpler to just override the toString() methods for this 
enumerated type?


- Robert Nettleton


On May 12, 2016, 11:31 a.m., Dharmesh Makwana wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47296/
> ---
> 
> (Updated May 12, 2016, 11:31 a.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
> Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
> Sebastian Toader.
> 
> 
> Bugs: AMBARI-16629
> https://issues.apache.org/jira/browse/AMBARI-16629
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> Patch contains
> 1) Authentication : change the API as per suggestion and support for roles in 
> authentication process.
> 2) resultSize property removed from collection class.
> 3) "Preview" issue fixed.
> 
> 
> Diffs
> -
> 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
>  0442cf9 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
>  a60402e 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
>  cc61127 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
>  8535039 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/view/VList.java
>  97226d2 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
>  79a414c 
>   
> ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
>  315d736 
> 
> Diff: https://reviews.apache.org/r/47296/diff/
> 
> 
> Testing
> ---
> 
> Tested in my local Ambari environment.
> 
> 
> Thanks,
> 
> Dharmesh Makwana
> 
>

Re: Review Request 47296: Authentication API changes along with role integration and few minor fixes.

2016-05-12 Thread Dharmesh Makwana 

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

(Updated May 12, 2016, 11:31 a.m.)


Review request for Ambari, Alejandro Fernandez, Andrew Onischuk, Don Bosco 
Durai, Oliver Szabo, Robert Nettleton, Sandor Magyari, Sumit Mohanty, and 
Sebastian Toader.


Changes
---

Refactored the classes to remove excess indentation.


Bugs: AMBARI-16629
https://issues.apache.org/jira/browse/AMBARI-16629


Repository: ambari


Description
---

Patch contains
1) Authentication : change the API as per suggestion and support for roles in 
authentication process.
2) resultSize property removed from collection class.
3) "Preview" issue fixed.


Diffs (updated)
-

  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/LogsMgr.java
 0442cf9 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/manager/UserConfigMgr.java
 a60402e 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/query/QueryGenerationBase.java
 cc61127 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/util/JSONUtil.java
 8535039 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/view/VList.java
 97226d2 
  
ambari-logsearch/ambari-logsearch-portal/src/main/java/org/apache/ambari/logsearch/web/security/LogsearchExternalServerAuthenticationProvider.java
 79a414c 
  
ambari-logsearch/ambari-logsearch-portal/src/main/resources/logsearch.properties
 315d736 

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


Testing
---

Tested in my local Ambari environment.


Thanks,

Dharmesh Makwana