Skip to content

Conversation

@diegolovison
Copy link
Collaborator

The goal of this test is to demonstrate two scenarios that are failing:

During the test we have the following log

16:34:51,699 INFO  (main) [i.h.s.WrkScenarioTest] Test duration: 49s
16:34:51,700 INFO  (main) [i.h.s.WrkScenarioTest] --- phase: calibration ---
16:34:51,703 INFO  (main) [i.h.s.WrkScenarioTest] StatusHistory{previousStatus=NOT_STARTED, currentStatus=RUNNING, when=2025-12-22T16:34:14.995-03:00[America/Sao_Paulo], threadId=1}
16:34:51,704 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:20.995-03:00[America/Sao_Paulo] elapsed: 6s -> StatusHistory{previousStatus=RUNNING, currentStatus=FINISHED, when=2025-12-22T16:34:20.995-03:00[America/Sao_Paulo], threadId=1}
16:34:51,704 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:22.995-03:00[America/Sao_Paulo] elapsed: 2s -> StatusHistory{previousStatus=FINISHED, currentStatus=TERMINATING, when=2025-12-22T16:34:22.995-03:00[America/Sao_Paulo], threadId=1}
16:34:51,704 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:33.909-03:00[America/Sao_Paulo] elapsed: 10s -> StatusHistory{previousStatus=TERMINATING, currentStatus=TERMINATED, when=2025-12-22T16:34:33.909-03:00[America/Sao_Paulo], threadId=56}
16:34:51,704 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:33.917-03:00[America/Sao_Paulo] elapsed: 0s -> StatusHistory{previousStatus=TERMINATED, currentStatus=STATS_COMPLETE, when=2025-12-22T16:34:33.917-03:00[America/Sao_Paulo], threadId=56}
16:34:51,704 INFO  (main) [i.h.s.WrkScenarioTest] --- phase: test ---
16:34:51,705 INFO  (main) [i.h.s.WrkScenarioTest] StatusHistory{previousStatus=NOT_STARTED, currentStatus=RUNNING, when=2025-12-22T16:34:33.917-03:00[America/Sao_Paulo], threadId=1}
16:34:51,705 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:43.917-03:00[America/Sao_Paulo] elapsed: 10s -> StatusHistory{previousStatus=RUNNING, currentStatus=FINISHED, when=2025-12-22T16:34:43.917-03:00[America/Sao_Paulo], threadId=1}
16:34:51,705 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:43.917-03:00[America/Sao_Paulo] elapsed: 0s -> StatusHistory{previousStatus=FINISHED, currentStatus=TERMINATING, when=2025-12-22T16:34:43.917-03:00[America/Sao_Paulo], threadId=1}
16:34:51,705 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:51.450-03:00[America/Sao_Paulo] elapsed: 7s -> StatusHistory{previousStatus=TERMINATING, currentStatus=TERMINATED, when=2025-12-22T16:34:51.450-03:00[America/Sao_Paulo], threadId=55}
16:34:51,705 INFO  (main) [i.h.s.WrkScenarioTest] 2025-12-22T16:34:51.450-03:00[America/Sao_Paulo] elapsed: 0s -> StatusHistory{previousStatus=TERMINATED, currentStatus=STATS_COMPLETE, when=2025-12-22T16:34:51.450-03:00[America/Sao_Paulo], threadId=55}

Between FINISHED and TERMINATED, What should we do with in-flight requests?

@franz1981
Copy link
Contributor

Just focus on one issue a time bud 🙏

@diegolovison
Copy link
Collaborator Author

diegolovison commented Dec 22, 2025

Just focus on one issue a time bud

Yes. The focus is WrkScenarioTest::wrk2Test.


Any idea about what we should do if in-flight requests?

Issue: Requests started in the calibration phase are still running when we switch to the test phase.

Impact: The test phase cannot process anything. Its 10-second time limit runs out while waiting for the old calibration work to finish.

Question: Should we make the system wait for all requests to finish before moving to the next phase?

@franz1981
Copy link
Contributor

Why it is waiting?
It is not using its own connections?

@franz1981
Copy link
Contributor

Try to understand why we have a calibration phase and what it is for; if is not needed or the reason why was there no longer valid, we should get rid of it.
Said that; usually phases should wait till the previous operations are completed: it is not like that?

@diegolovison
Copy link
Collaborator Author

Try to understand why we have a calibration phase and what it is for; if is not needed or the reason why was there no longer valid, we should get rid of it.

The calibration, imho, is valid to warm up the loader.

usually phases should wait till the previous operations are completed: it is not like that?

Ok. So you are expecting that.


The idea of this PR is not to fix any issue. Just create a reproducer for it.

@franz1981
Copy link
Contributor

franz1981 commented Dec 29, 2025

The calibration, imho, is valid to warm up the loader.

Yes and no. To be fair you don't know when is warmed up (it depends by both client and server hotspot or anything which the server is running..) so we should have an option to decide to not make it happen too.
It could be surprising to users which expect all requests to be within the steady state only (the layden people would noticed that..)

@franz1981
Copy link
Contributor

franz1981 commented Dec 29, 2025

The calibration, imho, is valid to warm up the loader.

Yes and no. To be fair you don't know when is warmed up so we should have an option to decide to not make it happen too (or decide the calibration duration, which is better IMHO).
It could be surprising to users which expect all requests to be within the steady state only (the layden people would noticed that..)

@franz1981
Copy link
Contributor

I'm not able to see the tests failing on my machine: check what's going on @diegolovison 🙏

test
}

public BenchmarkBuilder getBenchmarkBuilder(String name, String url, boolean enableHttp2, int connections,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public BenchmarkBuilder getBenchmarkBuilder(String name, String url, boolean enableHttp2, int connections,
public BenchmarkBuilder createBenchmarkBuilder(String name, String url, boolean enableHttp2, int connections,

synchronized (this) {
assert status == Status.NOT_STARTED : "Status is " + status;
status = Status.RUNNING;
this.changeStatus(Status.RUNNING);
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to use this. for the these...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed by e77b7c0

I removed those unrelated changes from this PR

}
}

public void setEnableWatchdog(boolean enableWatchdog) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems unrelated to this issue: what it is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed by e77b7c0

I removed those unrelated changes from this PR

return benchmark.getAbsolutePath();
}

public class TestStatistics implements StatisticsCollector.StatisticsConsumer {
Copy link
Contributor

Choose a reason for hiding this comment

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

TestStatistics is already on BaseScenarioTest - no need of a new class which does the same (if it's the same)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TestStatistics from BaseScenarioTest is storing only the metric.
The new TestStatistics are separated per phase, which is where we can show the issue in the assert.

Copy link
Contributor

Choose a reason for hiding this comment

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

cannot we unify them and have just one type?

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