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 missing method in ruby pipeline #15013

Merged
merged 1 commit into from Apr 20, 2023

Conversation

kaisecheng
Copy link
Contributor

@kaisecheng kaisecheng commented Apr 19, 2023

Release notes

Fixed ruby engine pipeline crash during shutdown

What does this PR do?

This PR adds the missing method worker_threads_draining? to ruby pipeline which is added in #13934 to java pipeline for log msg improvement.

As ruby engine is deprecated in Logstash 8, this method is added for backward compatibility.

Why is it important/What is the impact to the user?

Logstash runs in ruby engine and enables queue.drain got NoMethodError during shutdown. The ruby pipeline crashes.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • test it manually

How to test this PR locally

  1. logstash.yml
    queue.type: persisted
    queue.drain: true

  2. run the pipeline with ruby engine bin/logstash --java-execution false

input { stdin {} }
output {
  tcp {
    host => "localhost"
    port => "3000"
    ssl_enable => false
  }
}
  1. no need to start tcp server
  2. send event to stdin, eg "asdf" to accumulate events in PQ
  3. send SIGINT to shutdown logstash
  4. Logstash should not show NoMethodError

Related issues

Relates #13934
Closes #15010

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

@kaisecheng kaisecheng merged commit 71d64a9 into elastic:7.17 Apr 20, 2023
2 of 3 checks passed
@jsvd jsvd added the v7.17.10 label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants