gnodet-bot commented on code in PR #29480:
URL: https://github.com/apache/camel/pull/29480#discussion_r4513164757
##########
components/camel-chaos/src/main/java/org/apache/camel/component/chaos/ChaosManager.java:
##########
@@ -709,8 +711,109 @@ public Map<String, Object> finishExploration() {
throw new IllegalStateException("No exploration is running");
}
finish(now, ChaosExploration.Finish.FINISHED);
- return exploration.toJson(now);
+ return exploration.toJson(now, this::findRule, new
TreeMap<>(routes));
+ }
+ }
+
+ /**
+ * Begins an experiment of the running exploration (CAMEL-26774): a
hypothesis and the rules that test it, added
+ * together (all or none), with at least one expectation ({@code expect=})
to decide it. The counts per route are
+ * read now, so its end says what changed while it ran. One experiment is
open at a time.
+ *
+ * @param hypothesis what the experiment tests, in a few
words, such as "a failed refund is read
+ * again"
+ * @param rules the rules in their one-line text form,
separated by {@code ;}
+ * @return the experiment, as in the report
+ * @throws IllegalArgumentException when a rule is not valid, or no rule
has an expectation
+ * @throws IllegalStateException when no exploration is running, or an
experiment is open
+ */
+ public Map<String, Object> beginExperiment(String hypothesis, String
rules) {
+ List<String> texts = ChaosRuleParser.splitRules(rules);
+ if (texts.isEmpty()) {
+ throw new IllegalArgumentException("The chaos rule is empty");
+ }
+ List<ChaosRule.Builder> definitions = new ArrayList<>();
+ for (String text : texts) {
+ definitions.add(parse(text));
+ }
+ if (definitions.stream().allMatch(d -> d.expectRoute == null)) {
+ throw new IllegalArgumentException(
+ "An experiment needs an expectation (expect=) on one of
its rules to decide its hypothesis, such as"
+ + " expect=orders:failed<=0
within=1m");
+ }
+ synchronized (lock) {
+ long now = TimeSource.currentTimeMillis();
+ checkDeadline(now);
+ if (exploration == null || !exploration.isRunning()) {
+ throw new IllegalStateException("No exploration is running: an
experiment is part of one");
+ }
+ ChaosExperiment open = exploration.openExperiment();
+ if (open != null) {
+ throw new IllegalStateException("Experiment " + open.number +
" is open: end it first");
+ }
+ Map<String, RouteCounts> counts = new TreeMap<>(routes);
+ List<String> ids = add(definitions);
+ ChaosExperiment experiment =
exploration.beginExperiment(hypothesis, ids, counts, now);
+ LOG.info("Chaos experiment {} began with rules {}{}",
experiment.number, ids,
+ experiment.hypothesis != null ? ": " +
experiment.hypothesis : "");
+ return experiment.toJson(this::findRule, counts, now);
+ }
+ }
+
+ /**
+ * Ends the open experiment: what came of it is read (the difference of
the counts per route, the faults of each
+ * rule, the states of the expectations and the verdict), its rules are
removed, and it stays in the journal.
+ *
+ * @param note what the agent makes of it (optional)
+ * @return the experiment, as in the report
+ * @throws IllegalStateException when no experiment is open
+ */
+ public Map<String, Object> endExperiment(String note) {
+ synchronized (lock) {
+ long now = TimeSource.currentTimeMillis();
+ // an experiment open at the deadline was ended with the
exploration
+ checkDeadline(now);
+ ChaosExperiment open = exploration != null ?
exploration.openExperiment() : null;
+ if (open == null) {
+ throw new IllegalStateException("No experiment is open");
+ }
+ endExperiment(open, now, ChaosExperiment.End.ENDED, note);
+ return open.toJson(this::findRule, routes, now);
Review Comment:
⚠️ **Inconsistent defensive copy** — this call passes the live `routes` map,
while every other `toJson`/`end` call in this class wraps it in `new
TreeMap<>(routes)` (see `beginExperiment`, the private `endExperiment`, and
`report`). This works today only because `experiment.end()` already cached the
result, so `toJson()` returns early without touching `routes` — but that is
non-obvious coupling.
```suggestion
return open.toJson(this::findRule, new TreeMap<>(routes), now);
```
##########
components/camel-chaos/src/main/java/org/apache/camel/component/chaos/ChaosManager.java:
##########
@@ -709,8 +711,109 @@ public Map<String, Object> finishExploration() {
throw new IllegalStateException("No exploration is running");
}
finish(now, ChaosExploration.Finish.FINISHED);
- return exploration.toJson(now);
+ return exploration.toJson(now, this::findRule, new
TreeMap<>(routes));
+ }
+ }
+
+ /**
+ * Begins an experiment of the running exploration (CAMEL-26774): a
hypothesis and the rules that test it, added
+ * together (all or none), with at least one expectation ({@code expect=})
to decide it. The counts per route are
+ * read now, so its end says what changed while it ran. One experiment is
open at a time.
+ *
+ * @param hypothesis what the experiment tests, in a few
words, such as "a failed refund is read
+ * again"
+ * @param rules the rules in their one-line text form,
separated by {@code ;}
+ * @return the experiment, as in the report
+ * @throws IllegalArgumentException when a rule is not valid, or no rule
has an expectation
+ * @throws IllegalStateException when no exploration is running, or an
experiment is open
+ */
+ public Map<String, Object> beginExperiment(String hypothesis, String
rules) {
+ List<String> texts = ChaosRuleParser.splitRules(rules);
+ if (texts.isEmpty()) {
+ throw new IllegalArgumentException("The chaos rule is empty");
+ }
+ List<ChaosRule.Builder> definitions = new ArrayList<>();
+ for (String text : texts) {
+ definitions.add(parse(text));
+ }
+ if (definitions.stream().allMatch(d -> d.expectRoute == null)) {
+ throw new IllegalArgumentException(
+ "An experiment needs an expectation (expect=) on one of
its rules to decide its hypothesis, such as"
+ + " expect=orders:failed<=0
within=1m");
+ }
+ synchronized (lock) {
+ long now = TimeSource.currentTimeMillis();
+ checkDeadline(now);
+ if (exploration == null || !exploration.isRunning()) {
+ throw new IllegalStateException("No exploration is running: an
experiment is part of one");
+ }
+ ChaosExperiment open = exploration.openExperiment();
+ if (open != null) {
+ throw new IllegalStateException("Experiment " + open.number +
" is open: end it first");
+ }
+ Map<String, RouteCounts> counts = new TreeMap<>(routes);
+ List<String> ids = add(definitions);
+ ChaosExperiment experiment =
exploration.beginExperiment(hypothesis, ids, counts, now);
+ LOG.info("Chaos experiment {} began with rules {}{}",
experiment.number, ids,
+ experiment.hypothesis != null ? ": " +
experiment.hypothesis : "");
+ return experiment.toJson(this::findRule, counts, now);
+ }
+ }
+
+ /**
+ * Ends the open experiment: what came of it is read (the difference of
the counts per route, the faults of each
+ * rule, the states of the expectations and the verdict), its rules are
removed, and it stays in the journal.
+ *
+ * @param note what the agent makes of it (optional)
+ * @return the experiment, as in the report
+ * @throws IllegalStateException when no experiment is open
+ */
+ public Map<String, Object> endExperiment(String note) {
+ synchronized (lock) {
+ long now = TimeSource.currentTimeMillis();
+ // an experiment open at the deadline was ended with the
exploration
+ checkDeadline(now);
+ ChaosExperiment open = exploration != null ?
exploration.openExperiment() : null;
+ if (open == null) {
+ throw new IllegalStateException("No experiment is open");
+ }
+ endExperiment(open, now, ChaosExperiment.End.ENDED, note);
+ return open.toJson(this::findRule, routes, now);
+ }
+ }
+
+ // reads what came of the experiment, and then removes its rules; with the
lock
+ private int endExperiment(ChaosExperiment experiment, long now,
ChaosExperiment.End by, String note) {
+ experiment.end(this::findRule, new TreeMap<>(routes), now, by, note);
+ int count = 0;
+ for (String id : experiment.rules) {
+ int index = indexOf(id);
+ if (index >= 0) {
+ ChaosRule removed = rules.remove(index);
+ removed.removed(State.REMOVED, now);
+ remember(removed);
+ count++;
+ }
+ }
+ update();
+ LOG.info("Chaos experiment {} ended: {}", experiment.number,
+ experiment.toJson(this::findRule, routes,
now).getString("verdict"));
Review Comment:
⚠️ **Same inconsistency** — the `LOG.info` call just below the
`experiment.end(…, new TreeMap<>(routes), …)` call passes raw `routes` to
`toJson()`. Again safe today due to caching, but inconsistent with the pattern
used everywhere else.
```suggestion
experiment.toJson(this::findRule, new TreeMap<>(routes),
now).getString("verdict"));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
