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

Refactor trigger rule simulation lib #16645

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

carlpulley-da
Copy link
Contributor

@carlpulley-da carlpulley-da commented Apr 1, 2023

Refactor trigger rule simulation library as follows:

  • move library code into its own bazel build rule
  • refactor TriggerRuleSimulation so that code can be ran on async workloads (useful for trigger resource and multi-process trigger simulation tests)
  • refactor CatGenerators into a trait so that they may be reused across tests (again useful for trigger resource and multi-process simulation tests)


trait CatGenerators {

protected def packageId: PackageId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We allow call/use sites to set the package ID

applicationId,
triggerParties,
)
class SimulationContext {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SimulationContext allows core state to be used on a per test basis

@@ -63,20 +50,6 @@ class TriggerRuleSimulationLibTest
.copy(allowTriggerTimeouts = true, allowInFlightCommandOverflows = true)
)

def forAll[T](gen: Gen[T], sampleSize: Int = 100, parallelism: Int = 1)(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into TriggerRuleSimulationLib for reuse elsewhere

@carlpulley-da carlpulley-da requested a review from a team April 1, 2023 09:16
@carlpulley-da carlpulley-da marked this pull request as ready for review April 1, 2023 09:16
Comment on lines +48 to +49
private[this] val metricCountData = mutable.Map.empty[Set[UUID], mutable.Map[String, Long]]
private[this] val metricTimingData =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not notice that before but why are we using a mutable data structure here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this point offline and have decided to not alter the code here.

Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da left a comment

Choose a reason for hiding this comment

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

LGTM

@carlpulley-da carlpulley-da merged commit 1a58e9a into main Apr 3, 2023
@carlpulley-da carlpulley-da deleted the cjp/trigger-simulation-lib-refactor branch April 3, 2023 16:21
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