Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prober circular list #218

Merged
merged 338 commits into from Aug 12, 2019

Conversation

@Sanger2000
Copy link
Collaborator

commented Aug 7, 2019

Added in AbstractCircularLinkedListIterator and DefaultCircularLinkedListIterator to utils as well as refactoring of ProbingSequence and ProbingStep as well as their tests to reflect new structure.


This change is Reviewable

@googlebot googlebot added the cla: yes label Aug 7, 2019

@Sanger2000 Sanger2000 force-pushed the Sanger2000:prober-circular-list branch from c1435d1 to 0805bb0 Aug 8, 2019

Sanger2000 added some commits Jul 2, 2019

Refactored WebWhois to accomodate jianglai's suggested changes and mo…
…dified tests to reflect this refactoring

@Sanger2000 Sanger2000 force-pushed the Sanger2000:prober-circular-list branch from ae57c77 to 58a0c18 Aug 8, 2019

@jianglai jianglai self-requested a review Aug 9, 2019

@jianglai
Copy link
Member

left a comment

Reviewed 3 of 11 files at r1.
Reviewable status: 3 of 11 files reviewed, 1 unresolved discussion (waiting on @jianglai and @Sanger2000)


util/src/main/java/google/registry/util/AbstractCircularLinkedListIterator.java, line 22 at r1 (raw file):

Quoted 10 lines of code…
/**
 * Immutable class that support circular iteration through a group of elements of type {@param
 * <T>}.
 *
 * @param <T> - Element type stored in the iterator
 *
 * <p>Is an immutable object that, when built, creates a circular pointing
 * group of Entries, that allows for looped iteration. The first and last element in the closed loop
 * are also stored.</p>
 */

I think you may be over-engineering. Is it really necessary implement a Iterator interface and have an Entry class to serve as a wrapper? I think we can get along with just one class.

How about the following: You can create sublasses of CircularList and override the Builder of it. You can also add add(T... values) and add(Iterable<T> values) method to the AbstractBuilder.

public class CircularList<T> {
  private final T value;
  private CircularList<T> next;

  public CircularList(T value) {
    this.value = value;
  }

  public T get() {
    return value;
  }

  public CircularList<T> next() {
    return next;
  }

  protected void next(CircularList<T> next) {
    this.next = next;
  }

  static class Builder<T> extends AbstractBuilder<T, CircularList<T>> {
    @Override
    CircularList<T> create(T value) {
      return new CircularList<>(value);
    }
  }

  abstract static class AbstractBuilder<T, C extends CircularList<T>> {
    private C first;
    private C current;

    abstract C create(T value);

    public AbstractBuilder<T, C> add(T value) {
      C c = create(value);
      if (current == null) {
        first = c;
      } else {
        current.next(c);
      }
      current = c;
      return this;
    }

    public C build() {
      current.next(first);
      return first;
    }
  }
}

@jianglai jianglai self-requested a review Aug 9, 2019

@jianglai
Copy link
Member

left a comment

Reviewable status: 3 of 11 files reviewed, 1 unresolved discussion (waiting on @jianglai and @Sanger2000)


util/src/main/java/google/registry/util/AbstractCircularLinkedListIterator.java, line 22 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…
/**
 * Immutable class that support circular iteration through a group of elements of type {@param
 * <T>}.
 *
 * @param <T> - Element type stored in the iterator
 *
 * <p>Is an immutable object that, when built, creates a circular pointing
 * group of Entries, that allows for looped iteration. The first and last element in the closed loop
 * are also stored.</p>
 */

I think you may be over-engineering. Is it really necessary implement a Iterator interface and have an Entry class to serve as a wrapper? I think we can get along with just one class.

How about the following: You can create sublasses of CircularList and override the Builder of it. You can also add add(T... values) and add(Iterable<T> values) method to the AbstractBuilder.

public class CircularList<T> {
  private final T value;
  private CircularList<T> next;

  public CircularList(T value) {
    this.value = value;
  }

  public T get() {
    return value;
  }

  public CircularList<T> next() {
    return next;
  }

  protected void next(CircularList<T> next) {
    this.next = next;
  }

  static class Builder<T> extends AbstractBuilder<T, CircularList<T>> {
    @Override
    CircularList<T> create(T value) {
      return new CircularList<>(value);
    }
  }

  abstract static class AbstractBuilder<T, C extends CircularList<T>> {
    private C first;
    private C current;

    abstract C create(T value);

    public AbstractBuilder<T, C> add(T value) {
      C c = create(value);
      if (current == null) {
        first = c;
      } else {
        current.next(c);
      }
      current = c;
      return this;
    }

    public C build() {
      current.next(first);
      return first;
    }
  }
}

Note that you can get rid of DefaultCircularLinkedListIterator as well by doing this.

@jianglai jianglai self-requested a review Aug 9, 2019

@Sanger2000
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @jianglai)


util/src/main/java/google/registry/util/AbstractCircularLinkedListIterator.java, line 22 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Note that you can get rid of DefaultCircularLinkedListIterator as well by doing this.

Done.

@jianglai
Copy link
Member

left a comment

Reviewed 1 of 11 files at r1, 9 of 12 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Sanger2000)


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 77 at r3 (raw file):

 testToken = Mockito.mock(Token.class);

Can do done when the field is declared.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 152 at r3 (raw file):

times(1)

I think times(1) is the default. you can just call verify(mockStep).....

Also, we need to test everything that runStep does, including testing that the channels gets set on the protocol, and that the second step runs.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 177 at r3 (raw file):

    verify(mockStep, times(1)).generateAction(testToken);

Similar comments to the previous method.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 195 at r3 (raw file):

,

delete


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 197 at r3 (raw file):

assertThrows(NullPointerException.class, sequence::start);

I don't quite get this. Why is this a NullPointerException? It is never a good idea to expect a null pointer exception, in general. If we are testing a failure case that we think may happen, we should throw a more specific exception.

@Sanger2000
Copy link
Collaborator Author

left a comment

Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @jianglai)


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 77 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…
 testToken = Mockito.mock(Token.class);

Can do done when the field is declared.

Done.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 152 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…
times(1)

I think times(1) is the default. you can just call verify(mockStep).....

Also, we need to test everything that runStep does, including testing that the channels gets set on the protocol, and that the second step runs.

Done.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 177 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…
    verify(mockStep, times(1)).generateAction(testToken);

Similar comments to the previous method.

Done.catch (Exception e){
}


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 195 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…
,

delete

Done.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 197 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…
assertThrows(NullPointerException.class, sequence::start);

I don't quite get this. Why is this a NullPointerException? It is never a good idea to expect a null pointer exception, in general. If we are testing a failure case that we think may happen, we should throw a more specific exception.

Resolved to throw a custom exception.

@jianglai jianglai self-requested a review Aug 9, 2019

@jianglai
Copy link
Member

left a comment

Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @jianglai and @Sanger2000)


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 152 at r3 (raw file):

Previously, Sanger2000 (Aman Sanger) wrote…

Done.

I think you missed my point. When sequence.start() is called. We should verify that the current step generates an action, the action is called, the channel on the protocol is set, and that the next step is called -- basically checking that everything that ProbingSequence.runStep() does.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 177 at r3 (raw file):

Previously, Sanger2000 (Aman Sanger) wrote…

Done.catch (Exception e){
}

Not sure what this means...


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 197 at r3 (raw file):

I still don't 100% understand where the NullPointerException comes from,
Why would there be different threads? We are using an EmbeddedChannel, right? Everything should be run in the main thread.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 43 at r4 (raw file):

 * <p>On every test that runs the sequence, in order for the sequence to stop, we expect an
 * error to be thrown on calling token.next(), as it should return null, which will create a
 * {@link NullPointerException}, ending the sequence after the last step has been performed.</p>

I am debating if it is really a good idea to always continue to the next step regardless errors. Would it be better to restart from the first step?

Also, exactly where is this NullPointerException supposed to be thrown? Is it when token.next() is called on the null token? Doesn't that happen when you finish your second loop? If that is the case, shouldn't every step in the sequence be called twice?

ProbingSequence and tests modified to reflect addition of Unrecoverab…
…leStateException and restarts on failures
@Sanger2000
Copy link
Collaborator Author

left a comment

Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @jianglai)


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 152 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…

I think you missed my point. When sequence.start() is called. We should verify that the current step generates an action, the action is called, the channel on the protocol is set, and that the next step is called -- basically checking that everything that ProbingSequence.runStep() does.

Resolved with new, comprehensive tests.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 177 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…

Not sure what this means...

Meant done.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 197 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…

I still don't 100% understand where the NullPointerException comes from,
Why would there be different threads? We are using an EmbeddedChannel, right? Everything should be run in the main thread.

New changes resolve this by terminating the sequence using the custom exception discussed (UnrecoverableStateException).


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 43 at r4 (raw file):

Previously, jianglai (Lai Jiang) wrote…
 * <p>On every test that runs the sequence, in order for the sequence to stop, we expect an
 * error to be thrown on calling token.next(), as it should return null, which will create a
 * {@link NullPointerException}, ending the sequence after the last step has been performed.</p>

I am debating if it is really a good idea to always continue to the next step regardless errors. Would it be better to restart from the first step?

Also, exactly where is this NullPointerException supposed to be thrown? Is it when token.next() is called on the null token? Doesn't that happen when you finish your second loop? If that is the case, shouldn't every step in the sequence be called twice?

We stop this by having mock Tokens return other mock Tokens on calls to next().

@jianglai
Copy link
Member

left a comment

Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Sanger2000)


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 69 at r5 (raw file):

Quoted 7 lines of code…
  private void setFirst(ProbingSequence first) {




    this.first = first;
  }

There's no need for this setter. Itself is a private method. You can just set the field directly.


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 131 at r5 (raw file):

Quoted 11 lines of code…
    try {
      // Call the generated action.
      future = currentAction.call();
    } catch (Exception e) {
      // On error in calling action, log error and note an error.
      logger.atWarning().withCause(e).log("Error in Action Performed");

      // Restart the sequence at the very first step.
      restartSequence();
      return;
    }

Can we put this part into the previous try-catch block? The logics are very similar any way. We should decide if to restart the sequence or to abort depending on exceptions thrown in currentAction.call() as well.


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 150 at r5 (raw file):

Quoted 6 lines of code…
        // If not unrecoverable, we restart the sequence.
        if (!(f.cause() instanceof UnrecoverableStateException)) {
          restartSequence();
        }
        // Otherwise, we just terminate the full sequence.
        return;

This is contrary to the logic above. UnrecoverableStateException should result in termination.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 48 at r5 (raw file):

 * error to be thrown on calling token.next(), as it should return null, which will create a {@link
 * NullPointerException}, ending the sequence after the last step has been performed.</p>

This needs updating. It is no longer accurate.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 74 at r5 (raw file):

  private Channel mockChannel = Mockito.mock(Channel.class);

Why do we need a mocked channel? Wouldn't any channel work? You are not using the channel in any way, right?


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 237 at r5 (raw file):

generateAction(any(Token.class)

In general it is better to verify the actual argument other than just the class.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 243 at r5 (raw file):

any(Token.class))

Same here.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 293 at r5 (raw file):

FailureGenerating

After you put the token generation and action calling in the same try-catch block you don't need to test them separately any more. Or even if you do want to, you should write a helper function and just change when the unrecoverable exception is thrown, as pretty much everything else is the same in these tests.

@jianglai
Copy link
Member

left a comment

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Sanger2000)


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 30 at r5 (raw file):

 * <p>Inherits from {@link CircularList}</p>, with element type of
 * {@link ProbingStep} as the manner in which the sequence is carried out is analogous to the {@link
 * CircularList}

Can we make a note there that even though ProbingSequence extends CircularList, its builder overrides the default ones so that the sequence doesn't necessary loop back to the first element, e. g. in case of EPP it loops back to the second step?

Modified ProbingSequence and its tests to reflect action generation a…
…nd calling being put in the same try catch block
@Sanger2000
Copy link
Collaborator Author

left a comment

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jianglai)


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 30 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
 * <p>Inherits from {@link CircularList}</p>, with element type of
 * {@link ProbingStep} as the manner in which the sequence is carried out is analogous to the {@link
 * CircularList}

Can we make a note there that even though ProbingSequence extends CircularList, its builder overrides the default ones so that the sequence doesn't necessary loop back to the first element, e. g. in case of EPP it loops back to the second step?

Done.


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 69 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
  private void setFirst(ProbingSequence first) {




    this.first = first;
  }

There's no need for this setter. Itself is a private method. You can just set the field directly.

Done.


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 131 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
    try {
      // Call the generated action.
      future = currentAction.call();
    } catch (Exception e) {
      // On error in calling action, log error and note an error.
      logger.atWarning().withCause(e).log("Error in Action Performed");

      // Restart the sequence at the very first step.
      restartSequence();
      return;
    }

Can we put this part into the previous try-catch block? The logics are very similar any way. We should decide if to restart the sequence or to abort depending on exceptions thrown in currentAction.call() as well.

Done.


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 150 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
        // If not unrecoverable, we restart the sequence.
        if (!(f.cause() instanceof UnrecoverableStateException)) {
          restartSequence();
        }
        // Otherwise, we just terminate the full sequence.
        return;

This is contrary to the logic above. UnrecoverableStateException should result in termination.

It does. If it is not an instance of UnrecoverableStateException, it restarts the sequence. Otherwise, it just goes to return, and the sequence is terminated.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 48 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
 * error to be thrown on calling token.next(), as it should return null, which will create a {@link
 * NullPointerException}, ending the sequence after the last step has been performed.</p>

This needs updating. It is no longer accurate.

Done.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 74 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
  private Channel mockChannel = Mockito.mock(Channel.class);

Why do we need a mocked channel? Wouldn't any channel work? You are not using the channel in any way, right?

True, we can just use the same embedded channel that we save.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 237 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
generateAction(any(Token.class)

In general it is better to verify the actual argument other than just the class.

Done. Verified both to ensure total number of calls is expected and specific calls are as expected.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 243 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
any(Token.class))

Same here.

Done.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 293 at r5 (raw file):

Previously, jianglai (Lai Jiang) wrote…
FailureGenerating

After you put the token generation and action calling in the same try-catch block you don't need to test them separately any more. Or even if you do want to, you should write a helper function and just change when the unrecoverable exception is thrown, as pretty much everything else is the same in these tests.

Done.

@jianglai
Copy link
Member

left a comment

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Sanger2000)


prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 150 at r5 (raw file):

Previously, Sanger2000 (Aman Sanger) wrote…

It does. If it is not an instance of UnrecoverableStateException, it restarts the sequence. Otherwise, it just goes to return, and the sequence is terminated.

OK my bad I didn't see the negation.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 178 at r6 (raw file):

verify(mockStep).generateAction(any(Token.class));

You don't need this anymore. The next verification is more specific.


prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 184 at r6 (raw file):

 verify(secondStep).generateAction(any(Token.class));

Same here.

@jianglai
Copy link
Member

left a comment

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Sanger2000)

@Sanger2000 Sanger2000 merged commit df5f450 into google:master Aug 12, 2019

4 of 7 checks passed

code-review/reviewable 2 discussions left (Sanger2000)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kokoro Kokoro build finished
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.