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

Support env memory overrides to start-tc-server (daggy main) #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akomakom
Copy link
Contributor

@akomakom akomakom commented Oct 14, 2021

Currently -Xms2g -Xmx2g is hardcoded.

There are 3 PRs, a review of just this one is all I need.

@akomakom akomakom changed the title Support env memory overrides to start-tc-server Support env memory overrides to start-tc-server (daggy main) Oct 14, 2021
@jhouserizer
Copy link
Contributor

In discussion with Gary, we're having second thoughts about this...
I'm re-checking with the gateway team about what problem they were having with using JAVA_OPTS , which should override the hard-coded value.

@akomakom
Copy link
Contributor Author

I guess that should work:
https://stackoverflow.com/a/58384752/1735931

@jhouserizer
Copy link
Contributor

Apparently not (specifying JAVA_OPTS), though they are vague as to why, simply saying it fails to start. I guess we need to test for ourselves with existing BM image.

@akomakom
Copy link
Contributor Author

akomakom commented Oct 18, 2021

This produces the expected error:

$ docker run  --rm -it -v /junk/file:/licenses/license.key  -v /junk/file:/configs/tc-config.xml -e JAVA_OPTS="-Xmx500g" -e ACCEPT_EULA=Y   XXXXXXXX/terracotta/bigmemorymax-server:4.3.10.0.68

# There is insufficient memory for the Java Runtime Environment to continue.

and:

-e JAVA_OPTS="-Xmx100m -Xms200m"

Error occurred during initialization of VM
Initial heap size set to a larger value than the maximum heap size

@cljohnso
Copy link
Contributor

Is there any way to see what is actually being passed in? I suspect a quoting issue ...

@jhouserizer
Copy link
Contributor

Is there any way to see what is actually being passed in? I suspect a quoting issue ...

I keep asking them to be specific and they keep being vague. Trying yet again...

@GaryWKeim
Copy link
Contributor

Did you try using an --env-file like:

environment:
    - JAVA_OPTS=-Xmx256m -Xms128m

@mobasherul
Copy link
Contributor

mobasherul commented Nov 9, 2021

In discussion with Gary, we're having second thoughts about this... I'm re-checking with the gateway team about what problem they were having with using JAVA_OPTS , which should override the hard-coded value.

@jhouserizer In Dockerfile JAVA_OPTS is being used so if u pass it while running container it will cause issue-
ENV JAVA_OPTS "-Dcom.tc.productkey.path=$LICENSE_DIRECTORY/license.key -XX:+UnlockExperimentalVMOptions -XX:+UseCGroupMemoryLimitForHeap"

so if u pass explicitly JAVA_OPTS while running container it will override the JAVA_OPTS value defined in Dockerfile and I get this error-
specify it as a system property with -Dcom.tc.productkey.path=/path/to/key
2021-11-09 13:55:39,007 ERROR - License key not found
2021-11-09 13:55:39,007 ERROR - License key not found
org.terracotta.license.LicenseException: License key not found
at com.tc.license.LicenseManager.assertLicenseValid(LicenseManager.java:101)
at com.tc.server.EnterpriseServerFactory.verifyLicensePresent(EnterpriseServerFactory.java:30)
at com.tc.server.EnterpriseServerFactory.createServer(EnterpriseServerFactory.java:17)
at com.tc.server.TCServerMain.main(TCServerMain.java:42)
2021-11-09 13:55:39,010 ERROR - Thread:Thread[main,5,main] got an uncaught exception. calling CallbackOnExitDefaultHandlers.
org.terracotta.license.LicenseException: License key not found
at com.tc.license.LicenseManager.assertLicenseValid(LicenseManager.java:101)
at com.tc.server.EnterpriseServerFactory.verifyLicensePresent(EnterpriseServerFactory.java:30)
at com.tc.server.EnterpriseServerFactory.createServer(EnterpriseServerFactory.java:17)
at com.tc.server.TCServerMain.main(TCServerMain.java:42)

@mobasherul
Copy link
Contributor

Note that both codelines are doing this:

Argument for this approach?

the JAVA_OPTS contain some options I guess that is not supported in java11 i.e - TAB-8036 .

@akomakom
Copy link
Contributor Author

keeping discussion together:

I'm guessing that this is a non-breaking change. I placed JAVA_MEMORY_OPTS before JAVA_OPTS, which means that:

  • users who are currently overriding memory settings via JAVA_OPTS will still be able to (for docker, this means losing default values)
  • users can switch to JAVA_MEMORY_OPTS

Questions:

  1. Would it be better to rename JAVA_MEMORY_OPTS to JAVA_EXTRA_OPTS?
  2. Would it be better to have both JAVA_MEMORY_OPTS and JAVA_EXTRA_OPTS in addition to JAVA_OPTS?
  3. Would it be better to rework all of this in some cohesive way?

I ask because I can reasonably expect customers to also want to add arbitrary java tuning flags that aren't memory specific.

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.

5 participants