Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5714)

Issue 303660043: DCTM: Use a traverser for modified ACLs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 7 months ago by JohnL
Modified:
7 years, 7 months ago
Reviewers:
Brett Johnson, Srinivas
CC:
connector-cr_google.com
Visibility:
Public.

Description

Use a traverser for modified ACLs. * Extend AclTraverser and add a new getAcls helper to override in the new ModifiedAclTraverser subclass. * The checkpoint was set only when the getUpdateAcls loop exited. A first iteration failure would lead to a null checkpoint, and a later iteration failure when return the initial checkpoint, discarding all progress. * Avoid uses of continue in the loop, since they would bypass updating the checkpoint. Related changes: * To preserve the existing caching behavior of chronicle IDs, cache a new chronicle ID immediately after a cache miss, and remove it when a DfIdNotFoundException is caught. * Add a getter and setter for checkpoint in TraverserTemplate as a consistent way for the tests to setup an initial checkpoint and validate the final checkpoint. Other changes: * Move the initialization of groupsCheckpoint in GroupTraverser from the constructor to fillCollection, for consistency with the ACL traversers. Also, this cleanly separates the necessary wiring of the initial checkpoint through the constructors from the design pattern of having TraverserTemplate provide and ask for the checkpoint when needed.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Code review change and also remove continue from getAcls. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -63 lines) Patch
M src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java View 1 5 chunks +24 lines, -27 lines 0 comments Download
M src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java View 9 chunks +38 lines, -32 lines 0 comments Download
M test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java View 3 chunks +51 lines, -4 lines 0 comments Download

Messages

Total messages: 7
JohnL
7 years, 7 months ago (2016-09-23 07:31:29 UTC) #1
Brett Johnson
LGTM++ https://codereview.appspot.com/303660043/diff/1/src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/303660043/diff/1/src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java#newcode216 src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:216: IDfACL dmAcl; The scope of dmAcl can be ...
7 years, 7 months ago (2016-09-23 18:17:04 UTC) #2
Srinivas
LGTM
7 years, 7 months ago (2016-09-23 18:52:19 UTC) #3
JohnL
Code review change and also remove continue from getAcls.
7 years, 7 months ago (2016-09-23 19:32:44 UTC) #4
JohnL
On 2016/09/23 19:32:44, JohnL wrote: > Code review change and also remove continue from getAcls. ...
7 years, 7 months ago (2016-09-23 19:34:42 UTC) #5
Brett Johnson
LGTM, even better.
7 years, 7 months ago (2016-09-23 21:06:54 UTC) #6
JohnL
7 years, 7 months ago (2016-09-26 20:44:54 UTC) #7

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b