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

more information about mqtt performance impact considerations & update a file name #2170

Merged
merged 9 commits into from
Dec 18, 2023

Conversation

Imaanpreet
Copy link
Contributor

@Imaanpreet Imaanpreet commented Apr 27, 2023

Please cherry-pick my commits into:

  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (planned Satellite 6.13)
  • Foreman 3.4/Katello 4.6 (EL8 only)
  • Foreman 3.3/Katello 4.5 on EL7 & EL8 (Satellite 6.12 on EL8 only; planned orcharhino 6.4 on EL8 only)
  • Foreman 3.2/Katello 4.4 on EL7 & EL8
  • Foreman 3.1/Katello 4.3 on EL7 & EL8 (Satellite 6.11 EL7/8; orcharhino 6.3 on EL7/8)
  • For Foreman 3.0 or older, please create a separate PR.
  • We do not accept PRs for Foreman 2.4 or older.

@Imaanpreet
Copy link
Contributor Author

Imaanpreet commented Apr 27, 2023

Hello @adamruzicka : can you please review this PR?

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Some nitpicks inline, CC @adamlazik1 just in case

@Imaanpreet
Copy link
Contributor Author

@adamruzicka : can we merge this request before Satellite 6.13 GA ?

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

GHA currently fail due to (probably unintended) changes I'd assume. Overall, some text needs to be slightly reworked to align with Foreman documentation.

@adamruzicka
Copy link
Contributor

@adamruzicka : can we merge this request before Satellite 6.13 GA ?

I cannot merge anything at all here, so we'll see

+
[options="nowrap", subs="+quotes,verbatim,attributes"]
----
cat >/etc/systemd/system/mosquitto.service.d/limits.conf <<__EOF__
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is invalid because the installer will purge this file.

And even if it is, I think we need an issue to track removal of this workaround. eclipse/mosquitto#2674 isn't getting any traction so if this is needed, an installer issue to manage this is needed.

----

{SmartProxyServer} logs are in `/var/log/foreman-proxy/proxy.log`.
{SmartProxyServer} uses Webrick HTTP server (no httpd or Puma involved), so there is no simple way to increase its capacity
Copy link
Member

Choose a reason for hiding this comment

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

If this is a bottleneck, is there an issue opened?

Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

A couple of formal suggestions.

@ekohl ekohl added the Waiting on contributor Requires an action from the author label Aug 28, 2023
@ekohl
Copy link
Member

ekohl commented Aug 28, 2023

Adding the label Waiting on contributor because there are open requests for changes.

@Imaanpreet
Copy link
Contributor Author

Hello, I did the requested changes. Thanks. Please let me know when the entire review is done, will squash all the commits into one.

@ekohl
Copy link
Member

ekohl commented Sep 11, 2023

@Imaanpreet this is waiting on changes. It also fails to build right now.

@Imaanpreet
Copy link
Contributor Author

I will look at this today, last week I did some changes but there was some conflicts. I will try to resolve this asap. Thank you @ekohl.

@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Sep 21, 2023
@mjivraja
Copy link
Contributor

ref_system-requirements-for-tuning.adoc file not found in include. @Imaanpreet

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Please address @mjivraja's concerns.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author Needs re-review and removed Needs re-review Waiting on contributor Requires an action from the author labels Oct 12, 2023
@Imaanpreet
Copy link
Contributor Author

hello, I have renamed the file or included the file in this PR.
moreover, I am going to suggest some values for mqtt_rate_limit this week.

Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

@Imaanpreet Thank you! I've unresolved a couple of comments and added a couple more suggestions that need to be addressed.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Needs re-review labels Dec 6, 2023
@Lennonka
Copy link
Contributor

Lennonka commented Dec 6, 2023

When those comments will have been addressed, we will probably be ready to merge.

Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Dec 14, 2023
@Imaanpreet
Copy link
Contributor Author

Imaanpreet commented Dec 14, 2023

When those comments will have been addressed, we will probably be ready to merge.

requested changes have been applied. can I squash the commits?

Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

I can squash the commits on merge, so you don't have to worry about that.
However, we are not finished with the content yet, see my comments below, please.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Needs re-review labels Dec 14, 2023
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Dec 18, 2023
@Lennonka Lennonka merged commit 8dc3c9f into theforeman:master Dec 18, 2023
8 checks passed
@Lennonka
Copy link
Contributor

Lennonka commented Dec 18, 2023

@Imaanpreet I went ahead and finished this off for you. If you need futher updates, please file another PR.

Lennonka added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
Lennonka added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
Lennonka added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
Lennonka added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
Lennonka added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
Lennonka added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
Lennonka added a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
@Lennonka
Copy link
Contributor

Cherry-picked:

ianballou pushed a commit to ianballou/foreman-documentation that referenced this pull request Dec 19, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
ianballou pushed a commit that referenced this pull request Dec 20, 2023
Co-authored-by: Zuzana Lena Ansorgová <zuansorg@redhat.com>
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.

None yet

8 participants