|
|
Created:
8 years, 10 months ago by Srinivas Modified:
8 years, 10 months ago CC:
pjo, brett.michael.johnson_gmail.com, connector-cr_google.com Visibility:
Public. |
DescriptionSupport ACL updates and deletes.
In Documentum, updates to ACL objects are recorded in dm_audittrail_acl table. Changes made to query this table for updates and deletes of ACLs.
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updated SessionMock, UserMock, and GroupMock #
Total comments: 2
Patch Set 3 : Final LGTM Changes #
Total comments: 46
Patch Set 4 : Changes as per John Lacy's comments #
Total comments: 14
Patch Set 5 : Changes as per comments #
MessagesTotal messages: 20
Please review the changes.
Sign in to reply to this message.
https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2777: private class SessionMock { Please take updated SessionMock, UserMock, and GroupMock from the AclTestProxies, which actually use the database to resolve users and groups.
Sign in to reply to this message.
Updated SessionMock, UserMock, and GroupMock as suggested. Please review.
Sign in to reply to this message.
Only a few nits on the source code; I will wait until you update the tests (as per Brett's message) before reviewing the test code. https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:256: * can have it's own individual ACL applied to it. So need to send all Last sentence here is missing a word or two. For example, "So we need to send ..." or "So this method needs to send ..." https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:657: logger.entering("DocumentumAdaptor", "getDocIds"); The second parameter should read "getModifiedDocIds" https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:660: logger.exiting("DocumentumAdaptor", "getDocIds"); Here as well.
Sign in to reply to this message.
On 2015/05/27 14:39:26, myk wrote: > Only a few nits on the source code; I will wait until you update the tests (as > per Brett's message) before reviewing the test code. > Hmm - that was *supposed* to have been sent last night, but the codereview tools seems to have frozen up / not sent that until now. I will go ahead and review the test code now.
Sign in to reply to this message.
test code LGTM; please address the few nits in the source code before submitting.
Sign in to reply to this message.
https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:657: logger.entering("DocumentumAdaptor", "getDocIds"); On 2015/05/27 14:39:26, myk wrote: > The second parameter should read "getModifiedDocIds" I already fixed these in my version. So leave this to avoid conflicts.
Sign in to reply to this message.
https://codereview.appspot.com/242760043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2743: + DocumentumAcls.DATE_FORMAT + "'), timestamp)"); Why the dateFormat conversion?
Sign in to reply to this message.
LGTM Changes. https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:256: * can have it's own individual ACL applied to it. So need to send all On 2015/05/27 14:39:26, myk wrote: > Last sentence here is missing a word or two. For example, "So we need to send > ..." > or "So this method needs to send ..." Done. https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:657: logger.entering("DocumentumAdaptor", "getDocIds"); On 2015/05/27 14:39:26, myk wrote: > The second parameter should read "getModifiedDocIds" Done. https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:657: logger.entering("DocumentumAdaptor", "getDocIds"); On 2015/05/27 19:03:26, Brett wrote: > On 2015/05/27 14:39:26, myk wrote: > > The second parameter should read "getModifiedDocIds" > > I already fixed these in my version. So leave this to avoid conflicts. Ok, undone the changes. :) https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2777: private class SessionMock { On 2015/05/27 00:14:06, Brett wrote: > Please take updated SessionMock, UserMock, and GroupMock from the > AclTestProxies, which actually use the database to resolve users and groups. These don't really affect the tests here. https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2777: private class SessionMock { On 2015/05/27 00:14:06, Brett wrote: > Please take updated SessionMock, UserMock, and GroupMock from the > AclTestProxies, which actually use the database to resolve users and groups. Done. https://codereview.appspot.com/242760043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2743: + DocumentumAcls.DATE_FORMAT + "'), timestamp)"); On 2015/05/27 19:13:20, Brett wrote: > Why the dateFormat conversion? For timestamp comparison. time_stamp_utc is stored as timestamp.
Sign in to reply to this message.
On 2015/05/27 19:21:08, Srinivas wrote: > LGTM Changes. > > https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... > File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): > > https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:256: * can have > it's own individual ACL applied to it. So need to send all > On 2015/05/27 14:39:26, myk wrote: > > Last sentence here is missing a word or two. For example, "So we need to send > > ..." > > or "So this method needs to send ..." > > Done. > > https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... > File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java > (right): > > https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:657: > logger.entering("DocumentumAdaptor", "getDocIds"); > On 2015/05/27 14:39:26, myk wrote: > > The second parameter should read "getModifiedDocIds" > > Done. > > https://codereview.appspot.com/242760043/diff/1/src/com/google/enterprise/ada... > src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:657: > logger.entering("DocumentumAdaptor", "getDocIds"); > On 2015/05/27 19:03:26, Brett wrote: > > On 2015/05/27 14:39:26, myk wrote: > > > The second parameter should read "getModifiedDocIds" > > > > I already fixed these in my version. So leave this to avoid conflicts. > > Ok, undone the changes. :) > > https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... > File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java > (right): > > https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... > test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2777: > private class SessionMock { > On 2015/05/27 00:14:06, Brett wrote: > > Please take updated SessionMock, UserMock, and GroupMock from the > > AclTestProxies, which actually use the database to resolve users and groups. > > These don't really affect the tests here. > > https://codereview.appspot.com/242760043/diff/1/test/com/google/enterprise/ad... > test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2777: > private class SessionMock { > On 2015/05/27 00:14:06, Brett wrote: > > Please take updated SessionMock, UserMock, and GroupMock from the > > AclTestProxies, which actually use the database to resolve users and groups. > > Done. > > https://codereview.appspot.com/242760043/diff/20001/test/com/google/enterpris... > File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java > (right): > > https://codereview.appspot.com/242760043/diff/20001/test/com/google/enterpris... > test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2743: + > DocumentumAcls.DATE_FORMAT + "'), timestamp)"); > On 2015/05/27 19:13:20, Brett wrote: > > Why the dateFormat conversion? > > For timestamp comparison. time_stamp_utc is stored as timestamp. Committed to Documentum Adaptor https://github.com/googlegsa/documentum/commit/05907dc17070dd394519a6ff3368bc...
Sign in to reply to this message.
Message was sent while issue was closed.
The bar for an LGTM is frustratingly low. John L https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:58: static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss"; @VisibleForTesting https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:64: private static String aclModifiedDate; Statics generally come before non-statics, though it's not a style rule. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:262: public Map<DocId, Acl> getUpdateAcls() throws DfException { This is a peer of getAcl and uses the same machinery. I think it belongs just below getAcls (ordering the query helpers however it makes sense). https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:307: String whereBoundedClauseDateOnly = " and (time_stamp_utc > " This is cruft. Why do we need this? https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:322: logger.log(Level.FINER, "UpdateAcl query: {0}", queryStr.toString()); toString is redundant. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:329: private String getNOW() { s/getNOW/getNow/ if you keep this method. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:330: SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); Why not inline this method, and why not use DATE_FORMAT? https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:655: public void getModifiedDocIds(DocIdPusher pusher) throws IOException, This stuff belong much higher in the file, but I think Brett is making that change in his CL. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:683: throw new IOException("Error getting Acls", e); s/Acls/ACLs/. English and Java have different capitalization rules. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:685: dmSessionManager.release(dmSession); Is it OK to release null? https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2749: colName = "time_stamp_utc"; Why do you need to make this transformation, if the query has "time_stamp_utc as time_stamp_utc_str"? https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3002: assertEquals( ImmutableSet.of( Delete space after first (. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3009: } You should add a test calling getUpdateAcls twice in a row, where the second has a checkpoint and produces no results. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3012: public void testUpdateAclsWithSameChronicleId() throws DfException, Break before throws throughout. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3023: assertEquals(1, aclMap.size()); Redundant and blocks a better error message for debugging, throughout. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3040: assertEquals(0, aclMap.size()); assertEquals(ImmutableMap.of(), aclMap) produces a better error message for debugging. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3058: assertEquals( ImmutableSet.of( Delete space after first (. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3080: private String getDateString(int min) { s/min/minutes/ https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3080: private String getDateString(int min) { Maybe s/getDateString/getNowPlusMinutes/ or addMinutesToNow, etc.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:58: static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss"; On 2015/05/27 20:34:49, JohnL wrote: > @VisibleForTesting Done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:64: private static String aclModifiedDate; On 2015/05/27 20:34:49, JohnL wrote: > Statics generally come before non-statics, though it's not a style rule. Done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:262: public Map<DocId, Acl> getUpdateAcls() throws DfException { On 2015/05/27 20:34:49, JohnL wrote: > This is a peer of getAcl and uses the same machinery. I think it belongs just > below getAcls (ordering the query helpers however it makes sense). Done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:307: String whereBoundedClauseDateOnly = " and (time_stamp_utc > " On 2015/05/27 20:34:49, JohnL wrote: > This is cruft. Why do we need this? Removed this legacy where clause that is not required. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:307: String whereBoundedClauseDateOnly = " and (time_stamp_utc > " On 2015/05/27 20:34:49, JohnL wrote: > This is cruft. Why do we need this? Done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:322: logger.log(Level.FINER, "UpdateAcl query: {0}", queryStr.toString()); On 2015/05/27 20:34:49, JohnL wrote: > toString is redundant. Done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:329: private String getNOW() { On 2015/05/27 20:34:49, JohnL wrote: > s/getNOW/getNow/ if you keep this method. Removed the method https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:329: private String getNOW() { On 2015/05/27 20:34:49, JohnL wrote: > s/getNOW/getNow/ if you keep this method. Done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:330: SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); On 2015/05/27 20:34:49, JohnL wrote: > Why not inline this method, and why not use DATE_FORMAT? Done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java (right): https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAdaptor.java:655: public void getModifiedDocIds(DocIdPusher pusher) throws IOException, On 2015/05/27 20:34:49, JohnL wrote: > This stuff belong much higher in the file, but I think Brett is making that > change in his CL. As discussed, rolled back the changes to this file. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2749: colName = "time_stamp_utc"; On 2015/05/27 20:34:50, JohnL wrote: > Why do you need to make this transformation, if the query has "time_stamp_utc as > time_stamp_utc_str"? removed it https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2749: colName = "time_stamp_utc"; On 2015/05/27 20:34:50, JohnL wrote: > Why do you need to make this transformation, if the query has "time_stamp_utc as > time_stamp_utc_str"? Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3002: assertEquals( ImmutableSet.of( On 2015/05/27 20:34:50, JohnL wrote: > Delete space after first (. Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3009: } On 2015/05/27 20:34:50, JohnL wrote: > You should add a test calling getUpdateAcls twice in a row, where the second has > a checkpoint and produces no results. Added a test case for second call to getUpdateAcls() with no results. Already have a test with second call with results. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3009: } On 2015/05/27 20:34:50, JohnL wrote: > You should add a test calling getUpdateAcls twice in a row, where the second has > a checkpoint and produces no results. Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3012: public void testUpdateAclsWithSameChronicleId() throws DfException, On 2015/05/27 20:34:50, JohnL wrote: > Break before throws throughout. Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3023: assertEquals(1, aclMap.size()); On 2015/05/27 20:34:50, JohnL wrote: > Redundant and blocks a better error message for debugging, throughout. Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3040: assertEquals(0, aclMap.size()); On 2015/05/27 20:34:50, JohnL wrote: > assertEquals(ImmutableMap.of(), aclMap) produces a better error message for > debugging. Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3058: assertEquals( ImmutableSet.of( On 2015/05/27 20:34:50, JohnL wrote: > Delete space after first (. Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3080: private String getDateString(int min) { On 2015/05/27 20:34:50, JohnL wrote: > s/min/minutes/ Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3080: private String getDateString(int min) { On 2015/05/27 20:34:50, JohnL wrote: > Maybe s/getDateString/getNowPlusMinutes/ or addMinutesToNow, etc. Done.
Sign in to reply to this message.
A bug and some TODOs. John L https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:307: String whereBoundedClauseDateOnly = " and (time_stamp_utc > " On 2015/05/28 01:41:15, Srinivas wrote: > On 2015/05/27 20:34:49, JohnL wrote: > > This is cruft. Why do we need this? > > Removed this legacy where clause that is not required. Partially removed it, and left a bug: aclModifyId is null, which will format as "null" in the query, which is larger than most and maybe all object IDs (which are hex strings). Empty string might work, but aclModifyId = "0" here seems the most clear. You could do more cleanup, but the logic around this will change a little with Brett's Checkpoint class, so I would just patch the bug here. I thought the general practice for getModifiedDocIds in other connectors was to look backward a little bit when starting? Otherwise, if the connector exits and is restarted, we lose updates from the time it was down until getDocIds is called again. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:25: import com.documentum.com.IDfClientX; Restore blank line. Is this something automated doing this? https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:295: private IDfQuery makeUpdateAclQuery() { Sorry if I wasn't clear, this method should have gone with getUpdateAcls (most helpfully, next to makeAclQuery, I think). As an aside, maybe pick between Modify, Modified, and Update as the adjective of choice? https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:310: aclModifiedDate = new SimpleDateFormat(DATE_FORMAT).format(new Date()); Maybe a TODO after the Checkpoint class is introduced, but we could simply initialize these fields in the declarations. Also, this date is local time, which is something we didn't fix in v3. Worth a TODO here. time_stamp_utc is generated as UTC, independent of the TZ offset (it's always UTC in the database, whereas time_stamp might be in server time), but is converted using the offset to get the server time we see in the DQL results. So getting the right value for "now" is not straightforward. If we want some slack, we could just use "now -1 day" to account for UTC and the startup slack I mentioned in the other comment.
Sign in to reply to this message.
https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3012: public void testUpdateAclsWithSameChronicleId() throws DfException, On 2015/05/28 01:41:15, Srinivas wrote: > On 2015/05/27 20:34:50, JohnL wrote: > > Break before throws throughout. > > Done. It is just as effective and more concise to use throws Exception for the tests. Any exception that is not an expectedException will fail the test. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:310: aclModifiedDate = new SimpleDateFormat(DATE_FORMAT).format(new Date()); On 2015/05/28 03:11:53, JohnL wrote: > Maybe a TODO after the Checkpoint class is introduced, but we could simply > initialize these fields in the declarations. > > Also, this date is local time, which is something we didn't fix in v3. Worth a > TODO here. > > time_stamp_utc is generated as UTC, independent of the TZ offset (it's always > UTC in the database, whereas time_stamp might be in server time), but is > converted using the offset to get the server time we see in the DQL results. So > getting the right value for "now" is not straightforward. If we want some slack, > we could just use "now -1 day" to account for UTC and the startup slack I > mentioned in the other comment. My CL duplicates this DateFormat in DocumentumAdaptor, and the no-arg constructor to Checkpoint does the -1 day initialization. So the Checkpoint refactoring could maintain this checkpoint in DocumentumAdaptor, with an initial checkpoint of { null objectId, YESTERDAY }. https://codereview.appspot.com/242760043/diff/80001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/80001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2743: + DocumentumAcls.DATE_FORMAT + "'), timestamp)"); Your use of CONVERT with PARSEDATETIME is unnecessary. Your replacement of DATETOSTRING returns the timestamp in a slightly incorrect format equal to: "yyyy-MM-dd HH:mm:ss.S" Here is a simplified query rewrite that is not only correct, but also uses the PARSEDATETIME and FORMATDATETIME H2 functions to desired effect: query = query.replace("DATETOSTRING", "FORMATDATETIME") .replace("DATE(", "PARSEDATETIME(") .replace("yyyy-mm-dd hh:mi:ss", "yyyy-MM-dd HH:mm:ss");
Sign in to reply to this message.
Reaction to Brett's comments. John L https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3012: public void testUpdateAclsWithSameChronicleId() throws DfException, On 2015/05/28 04:57:05, Brett wrote: > On 2015/05/28 01:41:15, Srinivas wrote: > > On 2015/05/27 20:34:50, JohnL wrote: > > > Break before throws throughout. > > > > Done. > > It is just as effective and more concise to use throws Exception > for the tests. Any exception that is not an expectedException > will fail the test. Agreed. I hadn't paid attention to the fact that these are test methods. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:136: aclModifyId = dmAclCollection.getString("r_object_id"); Swap these two lines, to consistently refer to date followed by ID everywhere. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:310: aclModifiedDate = new SimpleDateFormat(DATE_FORMAT).format(new Date()); On 2015/05/28 04:57:05, Brett wrote: > On 2015/05/28 03:11:53, JohnL wrote: > > Maybe a TODO after the Checkpoint class is introduced, but we could simply > > initialize these fields in the declarations. > > > > Also, this date is local time, which is something we didn't fix in v3. Worth a > > TODO here. > > > > time_stamp_utc is generated as UTC, independent of the TZ offset (it's always > > UTC in the database, whereas time_stamp might be in server time), but is > > converted using the offset to get the server time we see in the DQL results. > So > > getting the right value for "now" is not straightforward. If we want some > slack, > > we could just use "now -1 day" to account for UTC and the startup slack I > > mentioned in the other comment. > > My CL duplicates this DateFormat in DocumentumAdaptor, and the no-arg > constructor to Checkpoint does the -1 day initialization. So the Checkpoint > refactoring could maintain this checkpoint in DocumentumAdaptor, with an > initial checkpoint of { null objectId, YESTERDAY }. That reminds me it's <date, ID>, and elsewhere I point out null is broken: it's { YESTERDAY, "0" }.
Sign in to reply to this message.
Please review the changes. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:307: String whereBoundedClauseDateOnly = " and (time_stamp_utc > " On 2015/05/28 03:11:53, JohnL wrote: > On 2015/05/28 01:41:15, Srinivas wrote: > > On 2015/05/27 20:34:49, JohnL wrote: > > > This is cruft. Why do we need this? > > > > Removed this legacy where clause that is not required. > > Partially removed it, and left a bug: aclModifyId is null, which will format as > "null" in the query, which is larger than most and maybe all object IDs (which > are hex strings). Empty string might work, but aclModifyId = "0" here seems the > most clear. > > You could do more cleanup, but the logic around this will change a little with > Brett's Checkpoint class, so I would just patch the bug here. > > I thought the general practice for getModifiedDocIds in other connectors was to > look backward a little bit when starting? Otherwise, if the connector exits and > is restarted, we lose updates from the time it was down until getDocIds is > called again. Set aclModifyId = "0", when aclModifiedDate is null or empty. Added to TODO for getModifiedDate to adjust for server tz offset. After the checkpoint class is done. https://codereview.appspot.com/242760043/diff/40001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:307: String whereBoundedClauseDateOnly = " and (time_stamp_utc > " On 2015/05/28 03:11:53, JohnL wrote: > On 2015/05/28 01:41:15, Srinivas wrote: > > On 2015/05/27 20:34:49, JohnL wrote: > > > This is cruft. Why do we need this? > > > > Removed this legacy where clause that is not required. > > Partially removed it, and left a bug: aclModifyId is null, which will format as > "null" in the query, which is larger than most and maybe all object IDs (which > are hex strings). Empty string might work, but aclModifyId = "0" here seems the > most clear. > > You could do more cleanup, but the logic around this will change a little with > Brett's Checkpoint class, so I would just patch the bug here. > > I thought the general practice for getModifiedDocIds in other connectors was to > look backward a little bit when starting? Otherwise, if the connector exits and > is restarted, we lose updates from the time it was down until getDocIds is > called again. Done. https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/40001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:3012: public void testUpdateAclsWithSameChronicleId() throws DfException, On 2015/05/28 06:39:34, JohnL wrote: > On 2015/05/28 04:57:05, Brett wrote: > > On 2015/05/28 01:41:15, Srinivas wrote: > > > On 2015/05/27 20:34:50, JohnL wrote: > > > > Break before throws throughout. > > > > > > Done. > > > > It is just as effective and more concise to use throws Exception > > for the tests. Any exception that is not an expectedException > > will fail the test. > > Agreed. I hadn't paid attention to the fact that these are test methods. Done. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java (right): https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:25: import com.documentum.com.IDfClientX; On 2015/05/28 03:11:53, JohnL wrote: > Restore blank line. Is this something automated doing this? Yes, this keeps automatically deleted, I keep changing it. Missed this one. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:25: import com.documentum.com.IDfClientX; On 2015/05/28 03:11:53, JohnL wrote: > Restore blank line. Is this something automated doing this? Done. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:136: aclModifyId = dmAclCollection.getString("r_object_id"); On 2015/05/28 06:39:34, JohnL wrote: > Swap these two lines, to consistently refer to date followed by ID everywhere. Done. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:295: private IDfQuery makeUpdateAclQuery() { On 2015/05/28 03:11:53, JohnL wrote: > Sorry if I wasn't clear, this method should have gone with getUpdateAcls (most > helpfully, next to makeAclQuery, I think). > > As an aside, maybe pick between Modify, Modified, and Update as the adjective of > choice? Done. https://codereview.appspot.com/242760043/diff/80001/src/com/google/enterprise... src/com/google/enterprise/adaptor/documentum/DocumentumAcls.java:310: aclModifiedDate = new SimpleDateFormat(DATE_FORMAT).format(new Date()); On 2015/05/28 03:11:53, JohnL wrote: > Maybe a TODO after the Checkpoint class is introduced, but we could simply > initialize these fields in the declarations. > > Also, this date is local time, which is something we didn't fix in v3. Worth a > TODO here. > > time_stamp_utc is generated as UTC, independent of the TZ offset (it's always > UTC in the database, whereas time_stamp might be in server time), but is > converted using the offset to get the server time we see in the DQL results. So > getting the right value for "now" is not straightforward. If we want some slack, > we could just use "now -1 day" to account for UTC and the startup slack I > mentioned in the other comment. Added TODO for server tz offset. https://codereview.appspot.com/242760043/diff/80001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java (right): https://codereview.appspot.com/242760043/diff/80001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2743: + DocumentumAcls.DATE_FORMAT + "'), timestamp)"); On 2015/05/28 04:57:05, Brett wrote: > Your use of CONVERT with PARSEDATETIME is unnecessary. > Your replacement of DATETOSTRING returns the > timestamp in a slightly incorrect format equal to: > "yyyy-MM-dd HH:mm:ss.S" > > Here is a simplified query rewrite that is not only > correct, but also uses the PARSEDATETIME and > FORMATDATETIME H2 functions to desired effect: > > query = query.replace("DATETOSTRING", "FORMATDATETIME") > .replace("DATE(", "PARSEDATETIME(") > .replace("yyyy-mm-dd hh:mi:ss", "yyyy-MM-dd HH:mm:ss"); I remember trying this without using "convert" function and didn't work, maybe missed something. Changed it now. https://codereview.appspot.com/242760043/diff/80001/test/com/google/enterpris... test/com/google/enterprise/adaptor/documentum/DocumentumAdaptorTest.java:2743: + DocumentumAcls.DATE_FORMAT + "'), timestamp)"); On 2015/05/28 04:57:05, Brett wrote: > Your use of CONVERT with PARSEDATETIME is unnecessary. > Your replacement of DATETOSTRING returns the > timestamp in a slightly incorrect format equal to: > "yyyy-MM-dd HH:mm:ss.S" > > Here is a simplified query rewrite that is not only > correct, but also uses the PARSEDATETIME and > FORMATDATETIME H2 functions to desired effect: > > query = query.replace("DATETOSTRING", "FORMATDATETIME") > .replace("DATE(", "PARSEDATETIME(") > .replace("yyyy-mm-dd hh:mi:ss", "yyyy-MM-dd HH:mm:ss"); Done.
Sign in to reply to this message.
LGTM. Please wait for Marc's review. John L
Sign in to reply to this message.
LGTM as well. Thank you!
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
On 2015/05/28 23:59:52, Brett wrote: > LGTM. Committed to Documentum Adaptor https://github.com/googlegsa/documentum/commit/535172274b71cdc8a264355b79a3d8...
Sign in to reply to this message.
|