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

docs: update sysmon to oshisysmon in reference docs #15258

Closed
wants to merge 6 commits into from

Conversation

317brian
Copy link
Contributor

@317brian 317brian commented Oct 25, 2023

#14359 in 27.0.0 added OshiSysMonitor support to Druid, and Druid deprecated support for SysMonitor.

The reference docs still refer to SysMonitor. Since we didn't provide any additional migration steps beyond saying to include the correct module, I think we only need to update the config names in the reference page.

If this change isn't needed and the config name is staying the same, we should probably note that instead so people don't get confused.

@317brian
Copy link
Contributor Author

@hardikbajaj would you mind taking a look and confirming that this change is needed since you opened the PR that added Oshi.

@hardikbajaj
Copy link
Contributor

Hey @317brian , the updated Documentation for OshiSysMonitor is merged and added in 28.0 milestone #14912. I think i missed to add these changes in that PR, it would be needed, but can we keep config for both SysMonitor and OshiSysMonitor, and mark SysMonitor as deprecated. We're doing this everywhere.

@317brian
Copy link
Contributor Author

Fixed. PTAL @hardikbajaj

Copy link
Contributor

@ektravel ektravel left a comment

Choose a reason for hiding this comment

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

One question regarding code font. Otherwise, looks good.

docs/configuration/index.md Outdated Show resolved Hide resolved
@317brian
Copy link
Contributor Author

@@hardikbajaj PTAL

Copy link
Contributor

@hardikbajaj hardikbajaj left a comment

Choose a reason for hiding this comment

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

@317brian One miner nit, rest LGTM. @abhishekagarwal87 can you also have a look here if this looks good to you

@317brian
Copy link
Contributor Author

317brian commented Nov 8, 2023

@317brian One miner nit, rest LGTM. @abhishekagarwal87 can you also have a look here if this looks good to you

Cool, thanks for taking a look! I think GitHub ate your feedback for the nit though. I don't see it 😬

@317brian
Copy link
Contributor Author

317brian commented Dec 5, 2023

Ping @abhishekagarwal87

@@ -106,7 +106,7 @@ There are four JVM parameters that we set on all of our services:
- The temp directory should not be volatile tmpfs.
- This directory should also have good read and write speed.
- Avoid NFS mount.
- The `org.apache.druid.java.util.metrics.SysMonitor` requires execute privileges on files in `java.io.tmpdir`. If you are using the system monitor, do not set `java.io.tmpdir` to `noexec`.
- The `org.apache.druid.java.util.metrics.OshiSysMonitor` and `org.apache.druid.java.util.metrics.SysMonitor` both require execute privileges on files in `java.io.tmpdir`. If you are using the system monitor, do not set `java.io.tmpdir` to `noexec`. Note that SysMonitor is deprecated. We recommend using OshiSysMonitor.
Copy link
Contributor

Choose a reason for hiding this comment

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

@hardikbajaj - do you know if the OshiSysMonitor needs this privilege?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the [email protected] list.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Mar 30, 2024
Copy link

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Apr 27, 2024
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