#11781292 Add support for non-incremental groups push

#11781292 Add support for non-incremental groups push

[sfruhwald, 2016-10-21 21:17:34]

Thank you!

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
File src/com/google/enterprise/adaptor/DocIdSender.java (right):

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/DocIdSender.java:217: boolean caseSensitive,
boolean incremental)
If we really need a public overloaded version of this method without
ExceptionHandler, we should make the handler as last parameter (more
conventional way)..

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/DocIdSender.java:292: if (!incremental && !new
GsaVersion(gsaVerString).isAtLeast("7.4.0-0")) {
Please extract GsaVersion into a local variable above #286 and just reuse it
here..

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
File src/com/google/enterprise/adaptor/WrapperAdaptor.java (right):

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/WrapperAdaptor.java:446: boolean
caseSensitive, ExceptionHandler exceptionHandler,
Same here: "If we really need a public overloaded version of this method without
ExceptionHandler, we should make the handler as last parameter (more
conventional way).."

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
File src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java (right):

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:68: str =
"Document 1002 says hello and banana strawberry ; I am secured document";
"strawberry; I am" + " a" + " secured document"?

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:71:
.setPermitUsers(Arrays.asList(
this line-break seems unnecessary

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:73:
//.setInheritFrom(new DocId("1001"))
?

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:80: 
please remove these spaces, this is Eclipse's fault..

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:96: 
spaces

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:101: new
HashMap<DocId, AuthzStatus>(ids.size() * 2);
fits on one line

https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:101: new
HashMap<DocId, AuthzStatus>(ids.size() * 2);
why *2? we are putting exactly ids.size() number of elements, then return as
unmodifiableMap

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/AccumulatingDocIdPusher.java (right):

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/AccumulatingDocIdPusher.java:72: boolean
caseSensitive, ExceptionHandler handler, boolean incremental)
Again, the optional handler should be the last param

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/DocIdSenderTest.java (right):

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/DocIdSenderTest.java:366: = new
ArrayList<List<Entry<GroupPrincipal, Collection<Principal>>>>();
I would prefer keeping Map.Entry here.. its name is more self-explanatory

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/DocIdSenderTest.java:431:
List<List<Map.Entry<GroupPrincipal, Collection<Principal>>>> goldenGroups
It's Ok, no need for sorry and no need for 3 lines.. :)

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/DocIdSenderTest.java:435:
List<Map.Entry<GroupPrincipal, Collection<Principal>>> tmp
Using all of these immutable types here is overkill, as we don't pass over the
reference of goldenGroups..

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/DocIdSenderTest.java:443:
<Map.Entry<GroupPrincipal, Collection<Principal>>>singletonList(
Do we really need the type specifier here? As
AbstractMap.SimpleImmutableEntry<K,V> implements Map.Entry<K,V> and
singletonList(T) returns T..

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/DocIdSenderTest.java:468: // Order of
iteration matters
I cannot see the importance of these comments as you are just using this to fill
up the golden groups with..

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/DocIdSenderTest.java:486: tmp.add(new
SimpleImmutableEntry<GroupPrincipal, Collection<Principal>>(
Using all of these immutable types here is overkill, as we don't pass over the
reference of goldenGroups..

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java (right):

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java:257: final String
goldenResponse
the naming of this variable is misleading, as it is actually a golden request..
:)

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java:267: +
"incremental\r\n"
If I understand correctly, incremental was the default action and if GSA version
is less than 7.4.0-0, we force incremental boolean to 'true' along with a
warning message. But here we include this parameter for pre-7.4.0-0 GSA what we
did not do before.. Is it safe (ignored by <7.4.0-0)?

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java:290: final String
goldenResponse
same misleading var name..

[imysak, 2016-10-21 22:21:08]

On 2016/10/21 21:17:34, sfruhwald wrote:
> Thank you!
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> File src/com/google/enterprise/adaptor/DocIdSender.java (right):
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/DocIdSender.java:217: boolean caseSensitive,
> boolean incremental)
> If we really need a public overloaded version of this method without
> ExceptionHandler, we should make the handler as last parameter (more
> conventional way)..
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/DocIdSender.java:292: if (!incremental &&
!new
> GsaVersion(gsaVerString).isAtLeast("7.4.0-0")) {
> Please extract GsaVersion into a local variable above #286 and just reuse it
> here..
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> File src/com/google/enterprise/adaptor/WrapperAdaptor.java (right):
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/WrapperAdaptor.java:446: boolean
> caseSensitive, ExceptionHandler exceptionHandler,
> Same here: "If we really need a public overloaded version of this method
without
> ExceptionHandler, we should make the handler as last parameter (more
> conventional way).."
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> File src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java (right):
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:68: str =
> "Document 1002 says hello and banana strawberry ; I am secured document";
> "strawberry; I am" + " a" + " secured document"?
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:71:
> .setPermitUsers(Arrays.asList(
> this line-break seems unnecessary
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:73:
> //.setInheritFrom(new DocId("1001"))
> ?
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:80: 
> please remove these spaces, this is Eclipse's fault..
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:96: 
> spaces
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:101: new
> HashMap<DocId, AuthzStatus>(ids.size() * 2);
> fits on one line
> 
>
https://codereview.appspot.com/317770043/diff/20001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/examples/AdaptorTemplate.java:101: new
> HashMap<DocId, AuthzStatus>(ids.size() * 2);
> why *2? we are putting exactly ids.size() number of elements, then return as
> unmodifiableMap
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> File test/com/google/enterprise/adaptor/AccumulatingDocIdPusher.java (right):
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/AccumulatingDocIdPusher.java:72: boolean
> caseSensitive, ExceptionHandler handler, boolean incremental)
> Again, the optional handler should be the last param
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> File test/com/google/enterprise/adaptor/DocIdSenderTest.java (right):
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/DocIdSenderTest.java:366: = new
> ArrayList<List<Entry<GroupPrincipal, Collection<Principal>>>>();
> I would prefer keeping Map.Entry here.. its name is more self-explanatory
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/DocIdSenderTest.java:431:
> List<List<Map.Entry<GroupPrincipal, Collection<Principal>>>> goldenGroups
> It's Ok, no need for sorry and no need for 3 lines.. :)
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/DocIdSenderTest.java:435:
> List<Map.Entry<GroupPrincipal, Collection<Principal>>> tmp
> Using all of these immutable types here is overkill, as we don't pass over the
> reference of goldenGroups..
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/DocIdSenderTest.java:443:
> <Map.Entry<GroupPrincipal, Collection<Principal>>>singletonList(
> Do we really need the type specifier here? As
> AbstractMap.SimpleImmutableEntry<K,V> implements Map.Entry<K,V> and
> singletonList(T) returns T..
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/DocIdSenderTest.java:468: // Order of
> iteration matters
> I cannot see the importance of these comments as you are just using this to
fill
> up the golden groups with..
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/DocIdSenderTest.java:486: tmp.add(new
> SimpleImmutableEntry<GroupPrincipal, Collection<Principal>>(
> Using all of these immutable types here is overkill, as we don't pass over the
> reference of goldenGroups..
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> File test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java (right):
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java:257: final
String
> goldenResponse
> the naming of this variable is misleading, as it is actually a golden
request..
> :)
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java:267: +
> "incremental\r\n"
> If I understand correctly, incremental was the default action and if GSA
version
> is less than 7.4.0-0, we force incremental boolean to 'true' along with a
> warning message. But here we include this parameter for pre-7.4.0-0 GSA what
we
> did not do before.. Is it safe (ignored by <7.4.0-0)?
> 
>
https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
> test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java:290: final
String
> goldenResponse
> same misleading var name..

:)
As I said it is not real CL, rather first draft version of implementation to
looking for.
AdaptorTemplate is out of scope of this CL, this is just addition code for
extended manual testing, that shouldn't be pushed into the repository.

I agree with reordering parameters.

Library should support JDK6 -
http://googlegsa.github.io/adaptor/docs/ReleaseProcess.html so many from this
boilerplate code still needed.

I will review your other comments in Monday.

Thanks

[imysak, 2016-10-21 23:53:35]

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
File test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java (right):

https://codereview.appspot.com/317770043/diff/20001/test/com/google/enterpris...
test/com/google/enterprise/adaptor/GsaFeedFileSenderTest.java:267: +
"incremental\r\n"
On 2016/10/21 21:17:34, sfruhwald wrote:
> If I understand correctly, incremental was the default action and if GSA
version
> is less than 7.4.0-0, we force incremental boolean to 'true' along with a
> warning message. But here we include this parameter for pre-7.4.0-0 GSA what
we
> did not do before.. Is it safe (ignored by <7.4.0-0)?

yes, it is safe, and present in documentation.