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

Resolve com.sun.aas.instanceName in JVM arguments (PAYARA-660) #683

Merged
merged 1 commit into from Mar 14, 2016

Conversation

pdudits
Copy link
Contributor

@pdudits pdudits commented Feb 26, 2016

com.sun.aas.instanceName is the only glassfish system property that will not resolve when passed as JVM argument, which is strange omission.

In my use case I redirect all logging to logback, and logback gets initialized earlier than the system property is set during appserver startup. Passing it in JVM arguments guarantees it is available right from the process start.

@payara-ci
Copy link
Contributor

Can one of the admins verify this patch?

@smillidge smillidge added the PR: CLA CLA submitted on PR by the contributor label Mar 9, 2016
@smillidge smillidge added this to the Payara Server 4.1.1.162 milestone Mar 9, 2016
@smillidge
Copy link
Contributor

Tagged as PAYARA-660 internally

@smillidge smillidge changed the title Resolve com.sun.aas.instanceName in JVM arguments Resolve com.sun.aas.instanceName in JVM arguments (PAYARA-660) Mar 9, 2016
@danPowell52
Copy link
Contributor

jenkins test please

@danPowell52
Copy link
Contributor

I have reviewed the changes and everything seems in order. I will merge once the test suite confirms.

@payara-ci
Copy link
Contributor

All tests have passed

danPowell52 added a commit that referenced this pull request Mar 14, 2016
Resolve com.sun.aas.instanceName in JVM arguments (PAYARA-660)
@danPowell52 danPowell52 merged commit 8b417fd into payara:master Mar 14, 2016
@danPowell52
Copy link
Contributor

We have reverted this issue for now because on second review we are unsure what this addition is achieving. Could you please provide clarification as to what your problem is and what your intended fix is?

We've debugged this area of code, and we're not sure what your "resolving" issue is. For the DAS, it is already resolved as server at this point, which your fix does not change. In addition, if you specify the instance name in the domain.xml, it will always resolve as this once it hits the resolve methods a few lines later on (regardless of your fix).

For an instance, the instance name is null by default, but it is resolved as what you pass in with -Dcom.sun.aas.instanceName from the asadmin script, or as what you specify in the instance's domain.xml file. This is the only point we see your fix making a difference: if you don't pass in anything from the asadmin script and don't specify the name in the config, your fix will grab the default name of the instance (the name it was created with) and resolve it to this. Again though, if you specify an instance name in the instance's domain.xml, it will override the instance name your fix resolves.

@pdudits
Copy link
Contributor Author

pdudits commented Mar 15, 2016

In my case I've got <jvm-options>-Dlogback.serviceName=${com.sun.aas.instanceName}</jvm-options> in my DAS as well as instance configs, that is used to determine the log folder location and additional property of the log events when they're passed to ELK stack.

Value for instanceName is not present in the map at point of process start, therefore the process is started with value of the property logback.serviceName equal to ${com.sun.aas.instanceName}.

The system property is eventually set, but it is too late for logging subsystem, that writes out log events directly after the process is started, and only resolves the property once, therefore I've put it in jvm-options directly.

Another example would be usage for -Dvisualvm.display.name=${com.sun.aas.instanceName} so that instances can be easily identified in VisualVM instead of hunting for their PIDs. This also does not work without the patch.

I'll check once again if the resolution did not work for other reason, but the problem definetely did not occur after this line was added.

@pdudits
Copy link
Contributor Author

pdudits commented Mar 15, 2016

No, in my debugging session, without the patch, the token ${com.sun.aas.instanceName} will not be replaced with respective instance name when used in jvm options, whereas any of other standard java properties, environment variables and SystemPropertyConstants.*_PROPERTY properties would.

@danPowell52
Copy link
Contributor

Ok, thanks for the information and prompt reply. I am afraid I misunderstood what you were doing.

@danPowell52
Copy link
Contributor

I have merged the fix back in, thanks for your contribution.

@pdudits pdudits deleted the resolve-instance-name branch May 20, 2019 06:46
Cousjava pushed a commit to Cousjava/Payara that referenced this pull request Aug 21, 2019
PAYARA-3881 - 5.191 Maintenance

Approved-by: Jonathan Coustick <jonathan.coustick@payara.fish>
arieki pushed a commit to arieki/Payara that referenced this pull request Nov 15, 2022
…reateSocket-method-to-fix-ldap-bug

FISH-6567: override createSocket method without arguments to fix ldap connection issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants