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

Remove JVM settings from docker compose #589

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

mehdihasan
Copy link
Contributor

@mehdihasan mehdihasan commented Nov 3, 2023

  • Remove JVM settings from the docker compose
  • Update Readme to clarify usage of JVM settings

fixes #580

@Bert-R
Copy link
Collaborator

Bert-R commented Nov 4, 2023

The change is good, in the sense that it reportedly solves the user issue.
However... I wonder whether we should go this way or take one more step: totally remove the -Xss option. Do we really have reason to 'make it normal' to tune down the stack memory?

I checked the default on my system through this command: java -XX:+PrintFlagsFinal -version | grep ThreadStackSize:

     intx CompilerThreadStackSize                  = 1024                                   {pd product} {default}
     intx ThreadStackSize                          = 1024                                   {pd product} {default}
     intx VMThreadStackSize                        = 1024                                   {pd product} {default}
openjdk version "17.0.9" 2023-10-17
OpenJDK Runtime Environment Temurin-17.0.9+9 (build 17.0.9+9)
OpenJDK 64-Bit Server VM Temurin-17.0.9+9 (build 17.0.9+9, mixed mode, sharing)

I said 'make it normal'. I can imagine that users in some specific circumstances want to tune down the stack memory, but in normal situations, I would favor leaving it to the VM, to prevent issues of the type we just saw.

@mehdihasan @davideicardi Makes sense?

@mehdihasan
Copy link
Contributor Author

mehdihasan commented Nov 4, 2023

I agree.

It has come to my attention and you may have also encountered the following message:

OpenJDK 64-Bit Server VM warning: Options -Xverify:none and -noverify were deprecated in JDK 13 and will likely be removed in a future release.

I would like to propose two potential courses of action:

Proposal 1: I recommend considering the removal of the 'noverify' option.

Proposal 2: Additionally, I would like to suggest the evaluation of removing all JVM tuning options, as they primarily offer optimization for individual use cases. This approach would empower users to apply their own preferred options or limitations based on their specific requirements.

Your thoughts @Bert-R @davideicardi ?

@Bert-R
Copy link
Collaborator

Bert-R commented Nov 4, 2023

Indeed. Let's move that environment variable to the README and shortly describe there how they can tweak the JVM settings.

Can you change your PR that way? Otherwise, I can make a PR on Tuesday.

* Remove JVM settings from the docker compose
* Update Readme to clarify usage of JVM settings

fixes obsidiandynamics#580
@mehdihasan
Copy link
Contributor Author

How about now? I tried to be as precise as possible in the README.

@davideicardi
Copy link
Collaborator

I agree to use the "default" settings whenever it is possible. The less customization of the JVM we have, the better it is for everyone.

Eventually, if we find some options to be useful in certain scenarios, we can add a specific documentation section explain when you should considering adding a specific paramenters.

@mehdihasan Just try to rename the PR title to something more specific. What about: "Remove default JVM settings from docker compose"?

@mehdihasan mehdihasan changed the title Update docker compose Remove default JVM settings from docker compose Nov 6, 2023
@mehdihasan mehdihasan changed the title Remove default JVM settings from docker compose Remove JVM settings from docker compose Nov 6, 2023
README.md Outdated Show resolved Hide resolved
@Bert-R Bert-R merged commit 80ccddc into obsidiandynamics:master Nov 6, 2023
1 check passed
@Bert-R
Copy link
Collaborator

Bert-R commented Nov 6, 2023

Thanks!

@mehdihasan mehdihasan deleted the 580 branch November 6, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kafdrop 4.0.0 Docker image doesn't start
3 participants