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

added executor_threads documentation #421

Merged
merged 4 commits into from
Aug 31, 2021

Conversation

111andre111
Copy link
Contributor

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

@111andre111
Copy link
Contributor Author

Here was a parallel one:
#409

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.

Just left a minimal suggestion

docs/index.asciidoc Outdated Show resolved Hide resolved
@andsel andsel requested a review from karenzone July 28, 2021 06:59
@111andre111
Copy link
Contributor Author

@andsel
What is the influcence of the draft PR #410
mentioned in #409?

@andsel
Copy link
Contributor

andsel commented Jul 28, 2021

that description is more extended, we could use that, and close that draft PR with this one

@andsel
Copy link
Contributor

andsel commented Jul 28, 2021

the #410 is some more deep, where the #409 and this are only a face of the problem it's trying to fix

@andsel
Copy link
Contributor

andsel commented Jul 29, 2021

Hi @111andre111 I've rebased to master branch fixing conflicts and improved description.

@111andre111
Copy link
Contributor Author

So I assume we can merge this one and #409 as well if there weren't duplicates plus I assume #410 still will need some investigation time, correct? @andsel

@andsel
Copy link
Contributor

andsel commented Aug 9, 2021

Yes correct @111andre111, but Changelog needs a fix

@111andre111
Copy link
Contributor Author

@andsel I have fixed now the Changelog

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Thank you for adding this important documentation. Nice job explaining it. I left some minor suggestions inline for your consideration.

docs/index.asciidoc Show resolved Hide resolved
@andsel andsel self-requested a review August 11, 2021 06:41
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, merge squashing the commits please, or ping me if you need I do it

@111andre111
Copy link
Contributor Author

@andsel Sorry if I didn't manage the squash. I always got an error message.

@andsel
Copy link
Contributor

andsel commented Aug 31, 2021

@111andre111 I've rebased to master there were some conflicts on CHANGELOG.md ao probably that prohibited the merge.
I don't publish yet this, I left the version x.y.z so that a next bugfix or feature also publish this.

@111andre111
Copy link
Contributor Author

Ah I see. I didn't see that. Thank you @andsel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants