Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-14 Thread Sridhar K 

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


Ship it!




Ship It!

- Sridhar K


On June 13, 2019, 9:08 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 13, 2019, 9:08 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/5/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-13 Thread Sarath Subramanian 

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

(Updated June 13, 2019, 2:08 p.m.)


Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
and Madhan Neethiraj.


Changes
---

addressed review comments.


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


Repository: atlas


Description
---

Fix stale transactions in atlas due to ATLAS-3246


Diffs (updated)
-

  
graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
 4dd641da7 
  repository/src/main/java/org/apache/atlas/services/MetricsService.java 
8fb68e926 
  repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
0c86aff2c 


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

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


Testing
---

No longer see stale transaction alerts in atlas logs.

Precommit: 
https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console


Thanks,

Sarath Subramanian

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-13 Thread Madhan Neethiraj 

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


Ship it!




Ship It!

- Madhan Neethiraj


On June 12, 2019, 9:04 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 9:04 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/3/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-13 Thread Madhan Neethiraj 

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




graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
Lines 409 (patched)


Shouldn't commit be done only after performRequestHandlerAction() is 
called?. Same for line #391 as well.

Please review.



repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
Lines 198 (patched)


Shouldn't graphRollback() be called on exception?

Same for line #198 as well.


- Madhan Neethiraj


On June 12, 2019, 9:04 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 9:04 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/3/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Sarath Subramanian 


> On June 12, 2019, 2:27 p.m., Sridhar K wrote:
> > repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
> > Line 167 (original), 167 (patched)
> > 
> >
> > The API does not match the normal tx api
> > 
> > normally, you would have 
> > 
> > open tx,
> > do some action,
> > commit or rollback tx.
> > 
> > I don't see open in the codeAm I missing some thing?

Transactions are automatically created with the first operation on the graph 
and closed explicitly using commit() or rollback() - 
https://docs.janusgraph.org/latest/tx.html


- Sarath


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


On June 12, 2019, 2:04 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 2:04 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/3/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Sridhar K 

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




repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
Line 167 (original), 167 (patched)


The API does not match the normal tx api

normally, you would have 

open tx,
do some action,
commit or rollback tx.

I don't see open in the codeAm I missing some thing?


- Sridhar K


On June 12, 2019, 9:04 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 9:04 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/3/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Sarath Subramanian 


> On June 12, 2019, 1:32 p.m., Ashutosh Mestry wrote:
> > repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
> > Lines 176 (patched)
> > 
> >
> > Should this be rollback?

Transactions can be terminated manually with commit() or rollback().


> On June 12, 2019, 1:32 p.m., Ashutosh Mestry wrote:
> > repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
> > Line 182 (original), 188 (patched)
> > 
> >
> > In general, if commit is done within catch, it may make sense to have 
> > it in finally.

commit is already applied in try block for each runnable thread, moving it to 
finally() would apply commit() twice which is not desirable.


> On June 12, 2019, 1:32 p.m., Ashutosh Mestry wrote:
> > repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
> > Lines 199 (patched)
> > 
> >
> > Rollback instead of commit?

Transactions can be terminated with commit() or rollback().


- Sarath


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


On June 12, 2019, 2:04 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 2:04 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/3/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Sarath Subramanian 


> On June 12, 2019, 1:13 p.m., Madhan Neethiraj wrote:
> > graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
> > Line 382 (original), 388 (patched)
> > 
> >
> > Consider using try/finally here as well - similar to #195.
> > 
> > Same for line #372 as well.

did not want to use try/finally here, since try block might not have multiple 
exits/exceptions. but added anyways.


- Sarath


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


On June 12, 2019, 2:04 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 2:04 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/3/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Sarath Subramanian 

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

(Updated June 12, 2019, 2:04 p.m.)


Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
and Madhan Neethiraj.


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


Repository: atlas


Description
---

Fix stale transactions in atlas due to ATLAS-3246


Diffs (updated)
-

  
graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
 4dd641da7 
  repository/src/main/java/org/apache/atlas/services/MetricsService.java 
8fb68e926 
  repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
0c86aff2c 


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

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


Testing
---

No longer see stale transaction alerts in atlas logs.

Precommit: 
https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console


Thanks,

Sarath Subramanian

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Ashutosh Mestry via Review Board 

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




repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
Lines 176 (patched)


Should this be rollback?



repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
Line 182 (original), 188 (patched)


In general, if commit is done within catch, it may make sense to have it in 
finally.



repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
Lines 199 (patched)


Rollback instead of commit?


- Ashutosh Mestry


On June 12, 2019, 8:02 p.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 8:02 p.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4dd641da7 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/2/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Sarath Subramanian 

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

(Updated June 12, 2019, 1:02 p.m.)


Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
and Madhan Neethiraj.


Changes
---

addressed review comments.


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


Repository: atlas


Description
---

Fix stale transactions in atlas due to ATLAS-3246


Diffs (updated)
-

  
graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
 4dd641da7 
  repository/src/main/java/org/apache/atlas/services/MetricsService.java 
8fb68e926 
  repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
0c86aff2c 


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

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


Testing
---

No longer see stale transaction alerts in atlas logs.

Precommit: 
https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console


Thanks,

Sarath Subramanian

Re: Review Request 70840: ATLAS-3276: Fix stale transactions in atlas due to ATLAS-3246 (Free-text search)

2019-06-12 Thread Madhan Neethiraj 

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




graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
Lines 181 (patched)


Consider using a try/finally.



repository/src/main/java/org/apache/atlas/services/MetricsService.java
Lines 149 (patched)


Is this commit() needed here, as the caller, getMetrics(), is annotated 
with @GraphTransaction?



repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
Lines 163 (patched)


is @GraphTransaction annotation needed here, as commit() is explicitly 
called in #171 and #177?



repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java
Lines 185 (patched)


is @GraphTransaction annotation needed here, as commit() is explicitly 
called in #195 and #201?


- Madhan Neethiraj


On June 12, 2019, 8:14 a.m., Sarath Subramanian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70840/
> ---
> 
> (Updated June 12, 2019, 8:14 a.m.)
> 
> 
> Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, 
> and Madhan Neethiraj.
> 
> 
> Bugs: ATLAS-3276
> https://issues.apache.org/jira/browse/ATLAS-3276
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Fix stale transactions in atlas due to ATLAS-3246
> 
> 
> Diffs
> -
> 
>   
> graphdb/janus/src/main/java/org/apache/atlas/repository/graphdb/janus/AtlasJanusGraphIndexClient.java
>  4b441bf6d 
>   repository/src/main/java/org/apache/atlas/services/MetricsService.java 
> 8fb68e926 
>   repository/src/main/java/org/apache/atlas/util/AtlasMetricsUtil.java 
> 0c86aff2c 
> 
> 
> Diff: https://reviews.apache.org/r/70840/diff/1/
> 
> 
> Testing
> ---
> 
> No longer see stale transaction alerts in atlas logs.
> 
> Precommit: 
> https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1199/console
> 
> 
> Thanks,
> 
> Sarath Subramanian
> 
>