Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Madhan Neethiraj]

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


Ship it!




Ship It!

- Madhan Neethiraj


On Sept. 13, 2017, 7:26 p.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62110/
> ---
> 
> (Updated Sept. 13, 2017, 7:26 p.m.)
> 
> 
> Review request for atlas and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-2115
> https://issues.apache.org/jira/browse/ATLAS-2115
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> ATLAS-1880 introduced regression in call timings for the basic search GET 
> implementation
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/model/discovery/SearchParameters.java 
> 972c11e3 
>   
> repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
>  0daab030 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java
>  a4a638af 
>   repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
> d5e39236 
>   repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
> fc973e6f 
> 
> 
> Diff: https://reviews.apache.org/r/62110/diff/5/
> 
> 
> Testing
> ---
> 
> Tested via scripts and network call timing in Chrome/Firefox.
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Apoorv Naik]

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

(Updated Sept. 13, 2017, 7:26 p.m.)


Review request for atlas and Madhan Neethiraj.


Changes
---

ADdressed review comments


Bugs: ATLAS-2115
https://issues.apache.org/jira/browse/ATLAS-2115


Repository: atlas


Description
---

ATLAS-1880 introduced regression in call timings for the basic search GET 
implementation


Diffs (updated)
-

  intg/src/main/java/org/apache/atlas/model/discovery/SearchParameters.java 
972c11e3 
  
repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
 0daab030 
  
repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java 
a4a638af 
  repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
d5e39236 
  repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
fc973e6f 


Diff: https://reviews.apache.org/r/62110/diff/5/

Changes: https://reviews.apache.org/r/62110/diff/4-5/


Testing
---

Tested via scripts and network call timing in Chrome/Firefox.


Thanks,

Apoorv Naik

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Madhan Neethiraj]

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




repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
Lines 286 (patched)


This comment doesn't look right. Please review and update.



repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java
Lines 67 (patched)


Since typeName is a single-value, 'in' predicate seems appropriate. Please 
review.



repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
Line 325 (original), 400 (patched)


this should be:
 return ((Collection)attrValue).contains(value);



repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
Lines 526 (patched)


this should be:
 return ((Collection)value).contains(attrVal);


- Madhan Neethiraj


On Sept. 13, 2017, 5:28 p.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62110/
> ---
> 
> (Updated Sept. 13, 2017, 5:28 p.m.)
> 
> 
> Review request for atlas and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-2115
> https://issues.apache.org/jira/browse/ATLAS-2115
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> ATLAS-1880 introduced regression in call timings for the basic search GET 
> implementation
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/model/discovery/SearchParameters.java 
> 972c11e3 
>   
> repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
>  0daab030 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java
>  a4a638af 
>   repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
> d5e39236 
>   repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
> fc973e6f 
> 
> 
> Diff: https://reviews.apache.org/r/62110/diff/4/
> 
> 
> Testing
> ---
> 
> Tested via scripts and network call timing in Chrome/Firefox.
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Apoorv Naik]

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

(Updated Sept. 13, 2017, 5:28 p.m.)


Review request for atlas and Madhan Neethiraj.


Changes
---

Added a BooleanPredicate alongwith new supported Operators (contains_any, 
contains_all)


Bugs: ATLAS-2115
https://issues.apache.org/jira/browse/ATLAS-2115


Repository: atlas


Description
---

ATLAS-1880 introduced regression in call timings for the basic search GET 
implementation


Diffs (updated)
-

  intg/src/main/java/org/apache/atlas/model/discovery/SearchParameters.java 
972c11e3 
  
repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
 0daab030 
  
repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java 
a4a638af 
  repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
d5e39236 
  repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
fc973e6f 


Diff: https://reviews.apache.org/r/62110/diff/4/

Changes: https://reviews.apache.org/r/62110/diff/3-4/


Testing
---

Tested via scripts and network call timing in Chrome/Firefox.


Thanks,

Apoorv Naik

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Apoorv Naik]

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

(Updated Sept. 11, 2017, 9:40 p.m.)


Review request for atlas and Madhan Neethiraj.


Bugs: ATLAS-2115
https://issues.apache.org/jira/browse/ATLAS-2115


Repository: atlas


Description
---

ATLAS-1880 introduced regression in call timings for the basic search GET 
implementation


Diffs (updated)
-

  
repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
 0daab030 
  
repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java 
a4a638af 
  repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
d5e39236 
  repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
fc973e6f 


Diff: https://reviews.apache.org/r/62110/diff/3/

Changes: https://reviews.apache.org/r/62110/diff/2-3/


Testing
---

Tested via scripts and network call timing in Chrome/Firefox.


Thanks,

Apoorv Naik

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Apoorv Naik]

> On Sept. 6, 2017, 11:17 p.m., Apoorv Naik wrote:
> > repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
> > Lines 329 (patched)
> > 
> >
> > I think this should be 
> > 
> > return (value instanceof Collection) && ((Collection) 
> > value).contains(attrVal);

Fixed in ATLAS-2117


- Apoorv


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


On Sept. 6, 2017, 11:11 p.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62110/
> ---
> 
> (Updated Sept. 6, 2017, 11:11 p.m.)
> 
> 
> Review request for atlas and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-2115
> https://issues.apache.org/jira/browse/ATLAS-2115
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> ATLAS-1880 introduced regression in call timings for the basic search GET 
> implementation
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
>  0daab030 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java
>  a4a638af 
>   repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
> d5e39236 
>   repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
> fc973e6f 
> 
> 
> Diff: https://reviews.apache.org/r/62110/diff/2/
> 
> 
> Testing
> ---
> 
> Tested via scripts and network call timing in Chrome/Firefox.
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Apoorv Naik]

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




repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java
Lines 329 (patched)


I think this should be 

return (value instanceof Collection) && ((Collection) 
value).contains(attrVal);


- Apoorv Naik


On Sept. 6, 2017, 11:11 p.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62110/
> ---
> 
> (Updated Sept. 6, 2017, 11:11 p.m.)
> 
> 
> Review request for atlas and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-2115
> https://issues.apache.org/jira/browse/ATLAS-2115
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> ATLAS-1880 introduced regression in call timings for the basic search GET 
> implementation
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
>  0daab030 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java
>  a4a638af 
>   repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
> d5e39236 
>   repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
> fc973e6f 
> 
> 
> Diff: https://reviews.apache.org/r/62110/diff/2/
> 
> 
> Testing
> ---
> 
> Tested via scripts and network call timing in Chrome/Firefox.
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Apoorv Naik]

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

(Updated Sept. 6, 2017, 11:11 p.m.)


Review request for atlas and Madhan Neethiraj.


Changes
---

Missed few changes in the previous patch plus addressed review comments


Bugs: ATLAS-2115
https://issues.apache.org/jira/browse/ATLAS-2115


Repository: atlas


Description
---

ATLAS-1880 introduced regression in call timings for the basic search GET 
implementation


Diffs (updated)
-

  
repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
 0daab030 
  
repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java 
a4a638af 
  repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
d5e39236 
  repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
fc973e6f 


Diff: https://reviews.apache.org/r/62110/diff/2/

Changes: https://reviews.apache.org/r/62110/diff/1-2/


Testing
---

Tested via scripts and network call timing in Chrome/Firefox.


Thanks,

Apoorv Naik

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Apoorv Naik]

> On Sept. 6, 2017, 10:28 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
> > Line 91 (original), 92 (patched)
> > 
> >
> > constructInMemoryPredicate() method was setting 'inMemoryPredicate'. 
> > Why this change? This breaks other callers of this method - like 
> > EntitySearchProcessor.java line #80. Please revert this change.

There's another use case where the code is almost similar with the only 
difference being graphAttributes instead of indexAttributes, hence the method 
was refactored to accomodate this change. I made sure that other callers don't 
break because of this change.


> On Sept. 6, 2017, 10:28 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
> > Line 98 (original), 99 (patched)
> > 
> >
> > Consider the following renaming for better readability:
> >   'allGraphQueryWithoutFilters' ==> 'entityGraphQueryWithTagFilter' 
> > (this query returns entity vertices)
> >   'allGraphQueryWithFilters'==> 'tagGraphQueryWithAttributesFilter' 
> > (this query returns classification vertices)

Will do.


> On Sept. 6, 2017, 10:28 p.m., Madhan Neethiraj wrote:
> > repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
> > Lines 207 (patched)
> > 
> >
> > this 'if' block is applicable even when indexQuery == null - if the 
> > block #192 - #198 is executed.
> > 
> > I think this condition should be:
> >  if (CollectionUtils.isNotEmpty(classificationVertices))

Looks like the patch missed this change. Will update the patch shortly.


- Apoorv


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


On Sept. 6, 2017, 6:07 a.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62110/
> ---
> 
> (Updated Sept. 6, 2017, 6:07 a.m.)
> 
> 
> Review request for atlas and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-2115
> https://issues.apache.org/jira/browse/ATLAS-2115
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> ATLAS-1880 introduced regression in call timings for the basic search GET 
> implementation
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
>  0daab030 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java
>  a4a638af 
>   repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
> d5e39236 
>   repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
> fc973e6f 
> 
> 
> Diff: https://reviews.apache.org/r/62110/diff/1/
> 
> 
> Testing
> ---
> 
> Tested via scripts and network call timing in Chrome/Firefox.
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>

Re: Review Request 62110: ATLAS-2115: Fix Regression on Basic search

[Madhan Neethiraj]

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




repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
Line 91 (original), 92 (patched)


constructInMemoryPredicate() method was setting 'inMemoryPredicate'. Why 
this change? This breaks other callers of this method - like 
EntitySearchProcessor.java line #80. Please revert this change.



repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
Line 98 (original), 99 (patched)


Consider the following renaming for better readability:
  'allGraphQueryWithoutFilters' ==> 'entityGraphQueryWithTagFilter' (this 
query returns entity vertices)
  'allGraphQueryWithFilters'==> 'tagGraphQueryWithAttributesFilter' 
(this query returns classification vertices)



repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
Lines 207 (patched)


this 'if' block is applicable even when indexQuery == null - if the block 
#192 - #198 is executed.

I think this condition should be:
 if (CollectionUtils.isNotEmpty(classificationVertices))


- Madhan Neethiraj


On Sept. 6, 2017, 6:07 a.m., Apoorv Naik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62110/
> ---
> 
> (Updated Sept. 6, 2017, 6:07 a.m.)
> 
> 
> Review request for atlas and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-2115
> https://issues.apache.org/jira/browse/ATLAS-2115
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> ATLAS-1880 introduced regression in call timings for the basic search GET 
> implementation
> 
> 
> Diffs
> -
> 
>   
> repository/src/main/java/org/apache/atlas/discovery/ClassificationSearchProcessor.java
>  0daab030 
>   
> repository/src/main/java/org/apache/atlas/discovery/EntitySearchProcessor.java
>  a4a638af 
>   repository/src/main/java/org/apache/atlas/discovery/SearchProcessor.java 
> d5e39236 
>   repository/src/main/java/org/apache/atlas/util/SearchPredicateUtil.java 
> fc973e6f 
> 
> 
> Diff: https://reviews.apache.org/r/62110/diff/1/
> 
> 
> Testing
> ---
> 
> Tested via scripts and network call timing in Chrome/Firefox.
> 
> 
> Thanks,
> 
> Apoorv Naik
> 
>