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

Make ContainerProxyTests more stable. #4289

Merged
merged 2 commits into from
Feb 14, 2019
Merged

Conversation

cbickel
Copy link
Contributor

@cbickel cbickel commented Feb 14, 2019

As the active ack is sent asynchronously in the invoker, it could happen, that the order of them is wrong in the ContainerProxyTest. This has been proven with the following debug information: #4278

This PR makes these tests more stable by not relying on the order of active acks.

Related issue and scope

  • I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

As the active ack is sent asynchronously in the invoker, it could happen, that the order of them is wrong in the ContainerProxyTest. This has been proven with the following debug information: apache#4278

This PR makes these tests more stable by not relying on the order of active acks.
@cbickel cbickel requested a review from rabbah February 14, 2019 08:53
@cbickel cbickel added review Review for this PR has been requested and yet needs to be done. testing heisenbug labels Feb 14, 2019
Copy link
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find, minor nit on saving some line sand boilerplate.

acker.calls.filter(_._2.annotations.get(WhiskActivation.initTimeAnnotation).isDefined)
initializedActivations should have size 1

val initRunActivation = initializedActivations.head._2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you already know that the size is 2 and you need both calls, could this be:

val (initRunActivation, runOnlyActivation) = {
  // false is sorted before true
  val sorted = acker.calls.sortBy(_._2.annotations.get(WhiskActivation.initTimeAnnotation).isEmpty)
  sorted(0)._2, sorted(1)._2
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. 👍

Copy link
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another nit for places where you only need one instance (didn't realize that's needed sorry)

acker.calls.filter(_._2.annotations.get(WhiskActivation.initTimeAnnotation).isDefined)
initializedActivations should have size 1

initializedActivations.head._2.annotations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the instances where you only need one of the calls:

val initRunActivation = acker.calls.find(_._2.annotations.get(WhiskActivation.initTimeAnnotation).isDefined).getOrElse(fail("..."))

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would keep consistency to the other tests that I've adapted, but then we need another check to verify that only one of the activations contains the init annotation.
So personally I'd prefer to use filter, checking the size of the result and using head.

@rabbah rabbah assigned markusthoemmes and unassigned rabbah Feb 14, 2019
Copy link
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alrighty then! Nice fix! 🙂

@markusthoemmes markusthoemmes merged commit 481a446 into apache:master Feb 14, 2019
@cbickel cbickel deleted the cpt branch February 14, 2019 15:39
@chetanmeh
Copy link
Member

Saw another set of failure related to ContainerProxy https://scans.gradle.com/s/36yxkcjvgad6y/tests/failed

@cbickel
Copy link
Contributor Author

cbickel commented Mar 5, 2019

@chetanmeh Thanks for the hint. But on the first view it looks to me, like there is a completely different root cause, compared to the problem that has been fixed here.

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
As the active ack is sent asynchronously in the invoker, it could happen, that the order of them is wrong in the ContainerProxyTest. This has been proven with the following debug information: apache#4278

This PR makes these tests more stable by not relying on the order of active acks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
heisenbug review Review for this PR has been requested and yet needs to be done. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants