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

#1700 Create custom Ocelot config file when instantiating steps during parallel execution #1703

Conversation

ggnaegi
Copy link
Member

@ggnaegi ggnaegi commented Sep 24, 2023

Fixes #1700

When instantiating steps, creating a custom ocelot config file name. Finally deleting the newly created config file when disposing the Step instance.

Proposed Changes

  • Since, by design, a new instance of Step is instantiated for each fact/theory, we need to ensure that the corresponding fact/theory only uses its own ocelot configuration file, without interfering with the configuration files of other facts and theories.

…Finally deleting the newly created config file when disposing the Step instance.
@raman-m
Copy link
Member

raman-m commented Sep 24, 2023

Wow! Cool!
Let's make 10 PR builds of 10 commits being added to this PR, to get some statistics on stability of this fix.

Just commit a small change of Ocelot core, not tests, and the 2nd commit should be revert commit of previous one.
So, there will be 5 commits and 5 revert-commits.

Good plan?

@ggnaegi
Copy link
Member Author

ggnaegi commented Sep 24, 2023

@raman-m no way, that's really funny ;-) 6 out of 10... I can't reproduce that on my machine

@ggnaegi
Copy link
Member Author

ggnaegi commented Sep 24, 2023

@raman-m Ok, a bit better now, 9 out of 11...

@raman-m raman-m force-pushed the bug/1700-unstable-test-should-reload-config-on-change branch from 1c97c39 to b17db75 Compare September 24, 2023 18:20
@ggnaegi
Copy link
Member Author

ggnaegi commented Sep 25, 2023

Open_circuit_should_not_effect_different_route

Forget about this test now. It is out of the scope of linked bug-issue. 🆗 I'm going to revi

It seems this fix works! 👍 What was the root cause of failing? I don't think it was random port finder... 🤣

@raman-m No it wasn't, but the random port finder code doesn't seem to be thread safe, since the concurrentbag would allow identical values to be stored and the method can be accessed by several threads concurrently

RaynaldM
RaynaldM previously approved these changes Sep 25, 2023
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Minor issues, but it is always good to improve the code.

test/Ocelot.AcceptanceTests/Steps.cs Outdated Show resolved Hide resolved
test/Ocelot.AcceptanceTests/Steps.cs Show resolved Hide resolved
test/Ocelot.AcceptanceTests/Steps.cs Show resolved Hide resolved
test/Ocelot.AcceptanceTests/Steps.cs Show resolved Hide resolved
test/Ocelot.AcceptanceTests/Steps.cs Show resolved Hide resolved
@ggnaegi
Copy link
Member Author

ggnaegi commented Sep 25, 2023

@raman-m imho ok to merge

@raman-m
Copy link
Member

raman-m commented Sep 25, 2023

raman-m
raman-m previously approved these changes Sep 25, 2023
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

+ Approved

test/Ocelot.AcceptanceTests/Steps.cs Show resolved Hide resolved
test/Ocelot.AcceptanceTests/Steps.cs Show resolved Hide resolved
@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs feedback Issue is waiting on feedback before acceptance labels Sep 25, 2023
@raman-m raman-m merged commit fdad15d into ThreeMammals:develop Sep 25, 2023
@raman-m
Copy link
Member

raman-m commented Sep 25, 2023

@ggnaegi Congrats!
We did that!
Thank you very much for your efforts during this Sunday's evening!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on bug Identified as a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unstable test "should reload config on change"
3 participants