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.

Sign up for GitHub

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

Jump to bottom

fix: check prometheus init status before use in batch processor (#8065) #8079

Merged
merged 4 commits into from
Oct 17, 2022
Merged

fix: check prometheus init status before use in batch processor (#8065) #8079

merged 4 commits into from
Oct 17, 2022

Conversation

monkeyDluffy6017
Copy link
Contributor

Description

check prometheus init status before use in batch processor

Fixes #8065

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

tzssangglass
tzssangglass previously approved these changes Oct 13, 2022
View reviewed changes
@tzssangglass
Copy link
Member

If the error of #8065 cannot be reproduced, then the fix does not work.

spacewander
spacewander reviewed Oct 13, 2022
View reviewed changes
t/plugin/kafka-logger.t



=== TEST 25: access
Copy link
Member

Choose a reason for hiding this comment

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

The test can pass even if I remove the change in the apisix/utils/batch-processor.lua.

Copy link

Choose a reason for hiding this comment

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

Can you show us the route information you configured?

Copy link
Contributor

Choose a reason for hiding this comment

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

It means to delete your change, the newly added test case can also pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can not reproduce the problem, but the test case can reach the code line where the user report, the test case is not a good one, i will try to reproduce again.

Copy link
Contributor Author

@monkeyDluffy6017 monkeyDluffy6017 Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have reproduced the problem, accessing the router with prometheus plugin disabled will cause the problem

@monkeyDluffy6017 monkeyDluffy6017 marked this pull request as draft October 14, 2022 04:42
@monkeyDluffy6017 monkeyDluffy6017 marked this pull request as ready for review October 14, 2022 07:03
@spacewander spacewander merged commit df64077 into apache:master Oct 17, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
@monkeyDluffy6017 @Liu-Junlin
fix: check prometheus init status before use in batch processor (apac…
2edf2c9 
…he#8065) (apache#8079)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HT404 HT404 HT404 left review comments

@tzssangglass tzssangglass tzssangglass approved these changes

@soulbird soulbird soulbird approved these changes

@spacewander spacewander spacewander approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: batch_process:push method failed in 2.15.0
5 participants
@monkeyDluffy6017 @tzssangglass @spacewander @soulbird @HT404