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

IntegrationTest: Let RandomKill and SyncChurn run in sequence #4588

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

eval-exec
Copy link
Collaborator

What problem does this PR solve?

Related changes

  • Integration Test: let RandomlyKill and SyncChurn run in sequnece

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note

None: Exclude this PR from the release note.

@eval-exec eval-exec force-pushed the exec/sequence-ci branch 2 times, most recently from a001c84 to 439ed15 Compare August 14, 2024 02:58
@eval-exec eval-exec marked this pull request as ready for review August 14, 2024 03:17
@eval-exec eval-exec requested a review from a team as a code owner August 14, 2024 03:17
@eval-exec eval-exec requested review from quake and removed request for a team August 14, 2024 03:17
while worker_running > 0 {
if max_time > 0 && start_time.elapsed().as_secs() > max_time {
// shutdown, specs running to long
workers.shutdown();
break;
}

if worker_running == 1 && !started_sequential {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems the last spec job may still have overlapping with the sequential spec job, suppose the msg is Notify::Start, means last job is starting executing, and we start workers.start_sequencial(), one sequential job will start, so there is a overlapping.

Copy link
Collaborator

@chenyukang chenyukang Aug 14, 2024

Choose a reason for hiding this comment

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

add a check && msg != Notify::Start ?
means the last job has just finished.

Copy link
Collaborator Author

@eval-exec eval-exec Aug 14, 2024

Choose a reason for hiding this comment

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

This seems the last spec job may still have overlapping with the sequential spec job, suppose the msg is Notify::Start, means last job is starting executing, and we start workers.start_sequencial(), one sequential job will start, so there is a overlapping.

workers.start_sequencial() will only be executed once.
After executing workers.start_sequencial(), the started_sequential variable will be true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it will be executed once, I mean when worker_running == 1 && !started_sequential is true, msg maybe Notify::Start, the last spec job is starting to run, not mean the last job has just finished, if we start sequential job at this time, there maybe a overlapping with last job?

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems it's ok, workers.start_sequencial() only set a flag.

Copy link
Collaborator Author

@eval-exec eval-exec Aug 14, 2024

Choose a reason for hiding this comment

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

When worker_running == 1, only one worker (the sequential worker) should be running, and all other parallel workers should have exited.

msg could be Notify::Start

If msg is Notify::Start, the sequential worker might be executing self.run_spec(spec.as_ref(), 0); at that moment.
A worker can execute only one spec at a time.
Therefore, there is no chance of overlap.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the last running worker must be the sequencial_worker...

@eval-exec eval-exec requested a review from chenyukang August 14, 2024 03:57
@eval-exec eval-exec added this pull request to the merge queue Aug 14, 2024
Merged via the queue into nervosnetwork:develop with commit bfcab0c Aug 14, 2024
39 checks passed
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