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

NMS-13176: add testing module including a benchmark app #28

Merged
merged 9 commits into from
Jun 17, 2021

Conversation

swachter
Copy link
Contributor

@swachter swachter commented Jun 11, 2021

Issue: https://issues.opennms.org/browse/NMS-13176

  • Adds the testing module that contains functionality to generate synthetic flows, write those flows to Kafka, or use them directly in a pipeline. In addition, a benchmark app can measure the throughput and processing time of pipeline runs.
  • Some minor changes to the pipeline to improve testability. E.g. the TimestampPolicyFactory must now be supplied when constructing a ReadFromKafka instance. This was necessary because the benchmark app needs a slightly patched timestamp policy factory.
  • A minor change how windowing parameters are passed around
  • Replaces distribution metrics for Kafka and ES drift by gauges. (Distribution metrics do not work with in the combination Beam/Flink/Prometheus.)

@swachter swachter requested a review from fooker June 11, 2021 09:45
Copy link
Member

@fooker fooker left a comment

Choose a reason for hiding this comment

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

Some minor nits and some proposals for the cardinality of some dimensions of the generated flows. I'm not sure if this is relevant for the current test but seems nice to have these available.

/**
* Determines if a flow is emitted or not.
*/
public abstract class Limiter {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use resilience4j RateLimiter instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would keep the current implementation. Its minimal and does exactly what is needed.

Copy link
Member

Choose a reason for hiding this comment

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

I would turn this around: Is there any reason not to use an implementation somebody else takes care for?

Copy link
Contributor Author

@swachter swachter Jun 16, 2021

Choose a reason for hiding this comment

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

resilience4j.RateLimiter seems to be an overkill here. RateLimiterConfig, RateLimiterRegistry, and RateLimiter are lots of ceremony. More severly: Looking through the documentation I did not see if there is a check(numberOfPermits) method.

Copy link
Member

Choose a reason for hiding this comment

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

There is reservePermission(numberOfPermits). If it returns 0 the call is permitted.
The registry is completely optional and the config is a temporary object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some time on resilience4j.RateLimiter and still think it is way too complicated. There seems to be no simple possibility to disable it. In addition, our limiter has not to be thread-safe. The custom Limiter implementation is simple. There is no maintenance burden. I see no benefit to use a dependency as a replacement.

Copy link
Member

@fooker fooker Jun 16, 2021

Choose a reason for hiding this comment

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

¯\(ツ)

swachter and others added 3 commits June 16, 2021 12:44
Co-authored-by: Dustin Frisch <fooker@lab.sh>
…nchmark.java

Co-authored-by: Dustin Frisch <fooker@lab.sh>
@swachter swachter requested a review from fooker June 16, 2021 12:01
swachter and others added 2 commits June 16, 2021 14:03
…heticFlowSource.java

Co-authored-by: Dustin Frisch <fooker@lab.sh>
@swachter swachter merged commit d286523 into master Jun 17, 2021
@swachter swachter deleted the jira/NMS-13176 branch June 17, 2021 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants