Revert erroneous change which drops user events on some completions.

[rabbah]

This fixes a bug introduced in a previous PR which drops user events on completion messages that do not carry a result (which is needed for user event generation).

Best to review this ignoring white space.

It appears there are no tests that caught the bug and so some tests are needed.
@sven-lange-last perhaps to verify the fix you can suggest a test to add 🙏

Description

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.

[chetanmeh]

Currently User Event feature does not have full coverage as it's disabled by default.

With support for embedded Kafka in Standalone we can now add an integration test where Invoker is started with user event enabled like being done in Activation Persister IT and then assert if messages are indeed being sent.

[codecov-io]

Codecov Report

Merging #4641 into master will decrease coverage by 5.63%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4641      +/-   ##
==========================================
- Coverage   84.62%   78.98%   -5.64%     
==========================================
  Files         183      183              
  Lines        8349     8347       -2     
  Branches      568      569       +1     
==========================================
- Hits         7065     6593     -472     
- Misses       1284     1754     +470

Impacted Files	Coverage Δ
...pache/openwhisk/core/invoker/InvokerReactive.scala	77.68% <0%> (+1.26%)	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	0% <0%> (-100%)	⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala	0% <0%> (-100%)	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	0% <0%> (-95.89%)	⬇️
...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala	0% <0%> (-94.74%)	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	0% <0%> (-92.6%)	⬇️
...e/database/cosmosdb/cache/ChangeFeedListener.scala	0% <0%> (-86.67%)	⬇️
...e/database/cosmosdb/cache/KafkaEventProducer.scala	0% <0%> (-76.48%)	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	0% <0%> (-74.08%)	⬇️
...a/org/apache/openwhisk/common/ExecutorCloser.scala	0% <0%> (-66.67%)	⬇️
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b536efe...5926d66. Read the comment docs.

[sven-lange-last]

PG 1 / 25

[chetanmeh]

I have pushed a IT test here which current fails on master but passes with this PR. If it seems fine I can push that to current PR branch

[rabbah]

Thanks @chetanmeh - I'll defer to you since it's more work for you.

[chetanmeh]

Pushed the tests to current PR branch

[rabbah]

Thanks @chetanmeh!

Note to @sven-lange-last when merging this PR please do not squash to preserve Chetan's commit.

[sven-lange-last]

I ran following UserEventTests in our build / test pipeline:

https://github.com/apache/openwhisk/blob/master/tests/src/test/scala/org/apache/openwhisk/common/UserEventTests.scala

The test failed once (PG 1 / 25) because it only waits for the user event message to arrive on Kafka for 10 seconds. So from my perspective, we should increase the maximum wait time to make the test a little more robust. A longer max poll time should not harm in most cases because the consumer.peek() will return as soon as messages could be received.

The next test run of UserEventTests (PG 1 / 26) succeeded with this PR. It always fails with current master.

In other words: this PR fixes the missing user events.

[sven-lange-last]

@rabbah thanks a lot for fixing this problem quickly. Your fix looks good to me. I verified that it re-enables user events as expected.

@chetanmeh thanks a lot for contributing a standalone test.

[chetanmeh]

Changes look fine and can now be merged

[sven-lange-last]

@steffenrost plans to improve the current UserEventTests as well as trying to a unit tests for user event sending.