RFR: 7455: Add support for jolokia JMX service connection [v11]

[Alex Macdonald]

On Thu, 7 Mar 2024 07:21:11 GMT, Martin Skarsaune <duke at openjdk.org> wrote:

>> Replaces parts of https://github.com/openjdk/jmc/pull/332
>
> Martin Skarsaune has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JMC-7455: Pick up  protocol extension and use it

Nice! I gave this a quick run and crawl through the debugger, and it looks like it's wired up correctly now (at least from what I ran).

These changes will also require the new `org.openjdk.jmc.jolokia` and `org.openjdk.jmc.jolokia.test` dependencies be added to `application/coverage/pom.xml` for test coverage runs.

Other than that I went through the code and made a couple comments.

application/org.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/AbstractCachedDescriptorProvider.java line 49:

> 47:  * changes.
> 48:  */
> 49: @SuppressWarnings("restriction")

As this will be added to JMC proper and not as an external extension, `org.openjdk.jmc.jolokia` could be added to the `org.openjdk.jmc.rjmx.descriptorprovider;x-friends` list at: https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.rjmx/META-INF/MANIFEST.MF#L19

application/org.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/AbstractCachedDescriptorProvider.java line 119:

> 117: 	 */
> 118: 	private void initialize() {
> 119: 

empty space

application/org.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/JmcJolokiaJmxConnectionProvider.java line 48:

> 46: 	public JMXConnector newJMXConnector(JMXServiceURL serviceURL, Map<String, ?> environment) throws IOException {
> 47: 		if (!"jolokia".equals(serviceURL.getProtocol())) { //$NON-NLS-1$
> 48: 			throw new MalformedURLException("I only serve Jolokia connections"); //$NON-NLS-1$

I think it'd be nicer if these strings were moved into `Messages.java`

application/org.openjdk.jmc.rjmx/src/main/java/org/openjdk/jmc/rjmx/internal/ProtocolInitializer.java line 2:

> 1: /*
> 2:  * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.

License year update 2023 -> 2024

application/org.openjdk.jmc.rjmx/src/main/java/org/openjdk/jmc/rjmx/internal/ServerHandle.java line 35:

> 33: package org.openjdk.jmc.rjmx.internal;
> 34: 
> 35: import java.io.IOException;

This file will need a license year update too

application/org.openjdk.jmc.rjmx/src/main/java/org/openjdk/jmc/rjmx/internal/ServerHandle.java line 147:

> 145: 					descriptor.getEnvironment());
> 146: 		} catch (IOException e) {
> 147: 			Logger.getLogger(getClass().getName()).log(Level.INFO, "Error attempting JMX protocol extensions", e);

Could use the `RJMXPlugin.getDefault().getLogger()` here and save on the import

application/tests/org.openjdk.jmc.jolokia.test/META-INF/MANIFEST.MF line 11:

> 9: Require-Bundle: org.junit,
> 10:  org.openjdk.jmc.jolokia,
> 11:  org.eclipse.osgi;bundle-version="3.16.200",

For osgi and hamcrest listed here, is it necessary for them to be targeted to specific versions?

application/tests/org.openjdk.jmc.jolokia.test/pom.xml line 68:

> 66: 					<includes>${test.includes}</includes>
> 67: 					<!-- Start jolokia on a random free port to avoid requiring specific ports to run test -->
> 68: 					<argLine>

Will need to add `${surefireArgLine}` in here for Jacoco to pick it up, once added to the `coverage/pom.xml`

core/org.openjdk.jmc.rjmx.common/src/main/java/org/openjdk/jmc/rjmx/common/internal/RJMXConnection.java line 591:

> 589: 
> 590: 	private void connectJmxConnector(JMXServiceURL serviceURL, Map<String, Object> env) throws IOException {
> 591: 		if (m_jmxc == null) {

This file will need a license year update too

-------------

PR Review: https://git.openjdk.org/jmc/pull/548#pullrequestreview-1922664846
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516329219
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516329913
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516337637
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516293491
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516294248
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516351189
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516353673
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516426135
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1516294427