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

Fix test failure #139

Merged
merged 2 commits into from
Dec 2, 2024
Merged

Conversation

Amalraj-Joseph-IBM
Copy link
Contributor

@Amalraj-Joseph-IBM
Copy link
Contributor Author

Hi @leochr ,
I just opened the PR as I thought this was relatively simple, and I thought I could make a contribution here.
I corrected the reference to the resource file, but even after that, a test case was failing, and I found it was a timeout when switching the service(/health) between UP and DOWN. So, I increased the Thread.sleep() value from 600, and I started to get consistent results at 1500.
To check if it is something related to my local setup, I checked out a previous commit (6ec559) and found 600 was working fine there. So I doubt some recent changes may have affected the app's performance (perhaps I'm wrong). Is it something to be concerned about?

@leochr
Copy link
Member

leochr commented Nov 28, 2024

@Amalraj-Joseph-IBM Thanks for taking a look at the issue and the PR. The changes look good to me.

As for the timeout, I am not sure. @fmhwong the Health-related tests work fine with mpMetrics-5.0 and mpConfig-3.0 but consistently fail with mpMetrics-5.1 and mpConfig-3.1 [when Thread.sleep(600) is used]. Is this a known performance degradation?

@fmhwong
Copy link
Member

fmhwong commented Nov 28, 2024

We haven't updated mpHealth for a while. No sure if the server was slow to update the configuration. @pgunapal Can you take a look?

@pgunapal
Copy link
Member

pgunapal commented Dec 2, 2024

Correct, there wasn't any recent changes to mpHealth. It's interesting that it's consistently failing with the newer versions of mpMetrics and mpConfig. I think its might be due to the fact, with HTTP metrics in mpMetrics-5.1. Might be related to this: OpenLiberty/open-liberty#30091

@Amalraj-Joseph-IBM
Copy link
Contributor Author

Thanks for the clarification.
So, can we merge the PR while leaving the above as a known issue?

@leochr
Copy link
Member

leochr commented Dec 2, 2024

Thank you all! I'll go ahead with approval and merge the PR.

@leochr leochr merged commit e5abd5c into OpenLiberty:main Dec 2, 2024
1 check failed
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.

4 participants