Add group push info (text only) to logs and dashboard

Add group push info (text only) to logs and dashboard

[myk, 2016-08-04 00:11:04]

Friendly reminder - thank you.

On 2016/07/30 02:47:43, myk wrote:

[sfruhwald, 2016-08-04 01:07:29]

Thank you, just a few slight comments from my side. LGTM otherwise.

https://codereview.appspot.com/308860044/diff/1/resources/com/google/enterpri...
File resources/com/google/enterprise/adaptor/resources/dashboard.js (right):

https://codereview.appspot.com/308860044/diff/1/resources/com/google/enterpri...
resources/com/google/enterprise/adaptor/resources/dashboard.js:183: var
curGroupPushStart = data.simpleStats.currentGroupPushStart;
Why not using the Boolean(..) constructor here too? (I see it is not used above
too, just don't know why and what is the benefit assigning it to a var
otherwise)

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
File src/com/google/enterprise/adaptor/DocIdSender.java (right):

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/DocIdSender.java:304:
journal.recordGroupPushInterrupted();
why not moving this out of the if-else block (above) as it is called in both
cases?

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
File src/com/google/enterprise/adaptor/Journal.java (right):

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Journal.java:169: long startTime =
currentGroupPushStart;
this temp variable seems unnecessary, if assignments are reordered:

this.lastSuccessfulGroupPushStart = currentGroupPushStart;
currentGroupPushStart = 0;

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Journal.java:182: throw new
IllegalStateException("Group push not already started");
.. not started yet? (but not sure if it's any better linguistically)

https://codereview.appspot.com/308860044/diff/1/test/com/google/enterprise/ad...
File test/com/google/enterprise/adaptor/JournalTest.java (right):

https://codereview.appspot.com/308860044/diff/1/test/com/google/enterprise/ad...
test/com/google/enterprise/adaptor/JournalTest.java:25: import
java.util.AbstractMap.SimpleEntry;
I would prefer importing only AbstractMap and using 'new
AbstractMap.SimpleEntry(..)' for better readability.

https://codereview.appspot.com/308860044/diff/1/test/com/google/enterprise/ad...
test/com/google/enterprise/adaptor/JournalTest.java:431: public void
testLastGroupPushStatus() {
Could you please add test cases for IllegalStateExceptions we throw in case push
has not been started at the time of calling Interrupted / Failed?

[myk, 2016-08-04 18:02:51]

Tried to explain "Boolean" in the dashboard.js; made all changes suggested by
Szabi.

https://codereview.appspot.com/308860044/diff/1/resources/com/google/enterpri...
File resources/com/google/enterprise/adaptor/resources/dashboard.js (right):

https://codereview.appspot.com/308860044/diff/1/resources/com/google/enterpri...
resources/com/google/enterprise/adaptor/resources/dashboard.js:183: var
curGroupPushStart = data.simpleStats.currentGroupPushStart;
On 2016/08/04 01:07:29, sfruhwald wrote:
> Why not using the Boolean(..) constructor here too? (I see it is not used
above
> too, just don't know why and what is the benefit assigning it to a var
> otherwise)

Done.

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
File src/com/google/enterprise/adaptor/DocIdSender.java (right):

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/DocIdSender.java:304:
journal.recordGroupPushInterrupted();
On 2016/08/04 01:07:29, sfruhwald wrote:
> why not moving this out of the if-else block (above) as it is called in both
> cases?

Done.  Definitely looks better outside of the "if"

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
File src/com/google/enterprise/adaptor/Journal.java (right):

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Journal.java:169: long startTime =
currentGroupPushStart;
On 2016/08/04 01:07:29, sfruhwald wrote:
> this temp variable seems unnecessary, if assignments are reordered:
> 
> this.lastSuccessfulGroupPushStart = currentGroupPushStart;
> currentGroupPushStart = 0;

Done.

https://codereview.appspot.com/308860044/diff/1/src/com/google/enterprise/ada...
src/com/google/enterprise/adaptor/Journal.java:182: throw new
IllegalStateException("Group push not already started");
On 2016/08/04 01:07:29, sfruhwald wrote:
> .. not started yet? (but not sure if it's any better linguistically)

I just copied an existing message and changed "Full" to "Group" -- but "not
started yet" is definitely better-sounding.  Made the change here and also line
192.

https://codereview.appspot.com/308860044/diff/1/test/com/google/enterprise/ad...
File test/com/google/enterprise/adaptor/JournalTest.java (right):

https://codereview.appspot.com/308860044/diff/1/test/com/google/enterprise/ad...
test/com/google/enterprise/adaptor/JournalTest.java:25: import
java.util.AbstractMap.SimpleEntry;
On 2016/08/04 01:07:29, sfruhwald wrote:
> I would prefer importing only AbstractMap and using 'new
> AbstractMap.SimpleEntry(..)' for better readability.

Done.

https://codereview.appspot.com/308860044/diff/1/test/com/google/enterprise/ad...
test/com/google/enterprise/adaptor/JournalTest.java:431: public void
testLastGroupPushStatus() {
On 2016/08/04 01:07:29, sfruhwald wrote:
> Could you please add test cases for IllegalStateExceptions we throw in case
push
> has not been started at the time of calling Interrupted / Failed?

Done -- with one additional case (two starts without ending/interrupting/failing
the first) -- at the bottom of this method.

[sfruhwald, 2016-08-04 18:13:21]

LGTM

[pjo, 2016-08-04 20:45:50]

THank you.

https://codereview.appspot.com/308860044/diff/20001/resources/com/google/ente...
File resources/com/google/enterprise/adaptor/resources/dashboard.js (right):

https://codereview.appspot.com/308860044/diff/20001/resources/com/google/ente...
resources/com/google/enterprise/adaptor/resources/dashboard.js:174:
data.simpleStats.lastSuccessfulGroupPushStart);
Does successfulGroupPushStart imply an entire push succeeded?  or does it imply
only that a group push started?

It seems like using lastSucessfulGroupPushEnd would remove ambiguity.

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

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/DocIdSender.java:263:
journal.recordGroupPushSuccessful();
is it really tho? given that nothing was sent? can you provide argument why this
is a good idea? (rather than skipping all calls to jounral)

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/DocIdSender.java:300: numGroups +=
batch.size();
IF we have a failedId then we didn't push batch.size() of them right?

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

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:36: private long
totalGroupPushes;
Annotate:
Equals to the sum of values in timesGroupPushed.

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:38: private Map<GroupPrincipal,
Integer> groupMembersPushed;
How about: totalGroupMembersPushed?

or at least annotate:
Accumulates total number of members pushed throughout all pushes of group.

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:39: private long
totalGroupMemberPushes;
Annotate:
Equals to the sum of values in groupMembersPushed.

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:215:
increment(timesNonGsaRequested, requested);
leave existing code alone, pls

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:307: private static void
groupIncrement(Map<GroupPrincipal, Integer> counts,
put method next to its use recordGroupPush

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:498: JournalSnapshot(Journal
journal, long currentTime, Stats[] timeStatsClone) {
Should we be synchronized on jounral instance in this ctor?

[myk, 2016-08-04 22:45:38]

Answered questions / made all requested changes.

https://codereview.appspot.com/308860044/diff/20001/resources/com/google/ente...
File resources/com/google/enterprise/adaptor/resources/dashboard.js (right):

https://codereview.appspot.com/308860044/diff/20001/resources/com/google/ente...
resources/com/google/enterprise/adaptor/resources/dashboard.js:174:
data.simpleStats.lastSuccessfulGroupPushStart);
On 2016/08/04 20:45:50, pjo wrote:
> Does successfulGroupPushStart imply an entire push succeeded?  or does it
imply
> only that a group push started?
> 
> It seems like using lastSucessfulGroupPushEnd would remove ambiguity.

Done - made it lastSucessfulGroupPushEnd (but it really did mean an entire push
succeeded before the change).

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

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/DocIdSender.java:263:
journal.recordGroupPushSuccessful();
On 2016/08/04 20:45:50, pjo wrote:
> is it really tho? given that nothing was sent? can you provide argument why
this
> is a good idea? (rather than skipping all calls to jounral)

When we call journal.recordGroupPushStarted(), we need to follow it with either
PushFailed(), PushInterrupted(), or PushSuccessful() before starting the
subsequent push.  Of these, Successful() seemed the best fit.  But after your
question, I think it's better to move the "journal.recordGroupPushStarted()" to
after this "if" -- that is, if we are asked to push an empty collection of
groups, we won't journal anything (just log) - we'll only record that a group
push started if there's at least one group to push.

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/DocIdSender.java:300: numGroups +=
batch.size();
On 2016/08/04 20:45:50, pjo wrote:
> IF we have a failedId then we didn't push batch.size() of them right?

OK - changed code to increment only if the batch was successful (with a TODO to
see if it makes sense to count a partial batch).

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

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:36: private long
totalGroupPushes;
On 2016/08/04 20:45:50, pjo wrote:
> Annotate:
> Equals to the sum of values in timesGroupPushed.
> 

Done.

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:38: private Map<GroupPrincipal,
Integer> groupMembersPushed;
On 2016/08/04 20:45:50, pjo wrote:
> How about: totalGroupMembersPushed?
> 
> or at least annotate:
> Accumulates total number of members pushed throughout all pushes of group.

Done (the annotation).

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:39: private long
totalGroupMemberPushes;
On 2016/08/04 20:45:50, pjo wrote:
> Annotate:
> Equals to the sum of values in groupMembersPushed.

Done.

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:215:
increment(timesNonGsaRequested, requested);
On 2016/08/04 20:45:50, pjo wrote:
> leave existing code alone, pls

I had made a change (which I later backed out) - I had not remembered to restore
the trailing blank, though.  It's back again (unchanged).

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:307: private static void
groupIncrement(Map<GroupPrincipal, Integer> counts,
On 2016/08/04 20:45:50, pjo wrote:
> put method next to its use recordGroupPush

Done.

https://codereview.appspot.com/308860044/diff/20001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:498: JournalSnapshot(Journal
journal, long currentTime, Stats[] timeStatsClone) {
On 2016/08/04 20:45:50, pjo wrote:
> Should we be synchronized on jounral instance in this ctor?

Makes sense to me.  Done.

[sfruhwald, 2016-08-05 00:07:55]

https://codereview.appspot.com/308860044/diff/40001/src/com/google/enterprise...
File src/com/google/enterprise/adaptor/Journal.java (right):

https://codereview.appspot.com/308860044/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:500: synchronized (journal) {
Maybe it would be more elegant to just make this constructor private and having
a synchronized createSnapshot method on Journal that returns with a Snapshot
instance?

[myk, 2016-08-05 17:29:13]

PTAL (both of you).  Reverting the change to add "synchronized" around the
JournalSnapshot constructor (with explanation below).

https://codereview.appspot.com/308860044/diff/40001/src/com/google/enterprise...
File src/com/google/enterprise/adaptor/Journal.java (right):

https://codereview.appspot.com/308860044/diff/40001/src/com/google/enterprise...
src/com/google/enterprise/adaptor/Journal.java:500: synchronized (journal) {
On 2016/08/05 00:07:55, sfruhwald wrote:
> Maybe it would be more elegant to just make this constructor private and
having
> a synchronized createSnapshot method on Journal that returns with a Snapshot
> instance?

I took a closer look after your comment, and it turns out that the constructor
is only called from two places: from Journal.getSnapshot() -- which is already
synchronized, so the most recent change of wrapping the whole method with
"synchronized" doesn't achieve anything -- and from StatRpcMethodTest.java
(where it's called from SnapshotMockJournal -- and there's only a single
instance of that class used in the tests, so that use doesn't need the
"synchronized" either).  I'm reverting the "synchronized (journal) { ... }"
wrapper, and adding @VisibleForTesting (as I would have made it private were it
not used in StatRpcMethodTest.class.

Voila - the constructor is once again "elegant."

[sfruhwald, 2016-08-05 17:51:11]

On 2016/08/05 17:29:13, myk wrote:
> PTAL (both of you).  Reverting the change to add "synchronized" around the
> JournalSnapshot constructor (with explanation below).
> 
>
https://codereview.appspot.com/308860044/diff/40001/src/com/google/enterprise...
> File src/com/google/enterprise/adaptor/Journal.java (right):
> 
>
https://codereview.appspot.com/308860044/diff/40001/src/com/google/enterprise...
> src/com/google/enterprise/adaptor/Journal.java:500: synchronized (journal) {
> On 2016/08/05 00:07:55, sfruhwald wrote:
> > Maybe it would be more elegant to just make this constructor private and
> having
> > a synchronized createSnapshot method on Journal that returns with a Snapshot
> > instance?
> 
> I took a closer look after your comment, and it turns out that the constructor
> is only called from two places: from Journal.getSnapshot() -- which is already
> synchronized, so the most recent change of wrapping the whole method with
> "synchronized" doesn't achieve anything -- and from StatRpcMethodTest.java
> (where it's called from SnapshotMockJournal -- and there's only a single
> instance of that class used in the tests, so that use doesn't need the
> "synchronized" either).  I'm reverting the "synchronized (journal) { ... }"
> wrapper, and adding @VisibleForTesting (as I would have made it private were
it
> not used in StatRpcMethodTest.class.
> 
> Voila - the constructor is once again "elegant."

That's perfect, thank you! LGTM again.

[pjo, 2016-08-08 23:45:14]

LGTM

-
- technology's compounding interest

On Fri, Aug 5, 2016 at 10:51 AM, <sfruhwald@google.com> wrote:

> On 2016/08/05 17:29:13, myk wrote:
>
>> PTAL (both of you).  Reverting the change to add "synchronized" around
>>
> the
>
>> JournalSnapshot constructor (with explanation below).
>>
>
>
> https://codereview.appspot.com/308860044/diff/40001/src/com/
> google/enterprise/adaptor/Journal.java
>
>> File src/com/google/enterprise/adaptor/Journal.java (right):
>>
>
>
> https://codereview.appspot.com/308860044/diff/40001/src/com/
> google/enterprise/adaptor/Journal.java#newcode500
>
>> src/com/google/enterprise/adaptor/Journal.java:500: synchronized
>>
> (journal) {
>
>> On 2016/08/05 00:07:55, sfruhwald wrote:
>> > Maybe it would be more elegant to just make this constructor private
>>
> and
>
>> having
>> > a synchronized createSnapshot method on Journal that returns with a
>>
> Snapshot
>
>> > instance?
>>
>
> I took a closer look after your comment, and it turns out that the
>>
> constructor
>
>> is only called from two places: from Journal.getSnapshot() -- which is
>>
> already
>
>> synchronized, so the most recent change of wrapping the whole method
>>
> with
>
>> "synchronized" doesn't achieve anything -- and from
>>
> StatRpcMethodTest.java
>
>> (where it's called from SnapshotMockJournal -- and there's only a
>>
> single
>
>> instance of that class used in the tests, so that use doesn't need the
>> "synchronized" either).  I'm reverting the "synchronized (journal) {
>>
> ... }"
>
>> wrapper, and adding @VisibleForTesting (as I would have made it
>>
> private were it
>
>> not used in StatRpcMethodTest.class.
>>
>
> Voila - the constructor is once again "elegant."
>>
>
> That's perfect, thank you! LGTM again.
>
> https://codereview.appspot.com/308860044/
>