You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2016/06/22 11:03:11 UTC
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/2145
[FLINK-4087] [metrics] Improved JMX port handling
This PR modifies the JMX port handling.
Instead of setting a specific configured port on JVM start-up we use an ephemeral port for the JVM's JMXServer.
Inside the JVM we then spin-up another JMXServer which users can connect to. This JMXServer picks a port from a configured port range; iterating from start to end until it finds an open one.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink metrics_jmx_ports
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2145.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2145
----
commit c91ee7cb8cd7f91e33208f31e55e29085302ff59
Author: zentol <ch...@apache.org>
Date: 2016-06-17T13:47:53Z
[FLINK-4087] [metrics] Improved JMX port handling
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68043366
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
+ int port = Integer.parseInt(ports[0]);
+ jmxServer = new JMXServer(port);
+ try {
+ jmxServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Could not start JMX server on port " + port + ".");
+ }
+ return jmxServer;
+ } else { //port range was configured
+ int start = Integer.parseInt(ports[0]);
+ int end = Integer.parseInt(ports[1]);
+ while (true) {
+ try {
+ jmxServer = new JMXServer(start);
+ jmxServer.start();
+ LOG.info("Starting JMX on port " + start + ".");
--- End diff --
I mean it could also be interesting to see which ports where tried to start the JMX server on. Maybe as a debug logging statement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2145
Good work @zentol. I mostly had general questions for my understanding. Before merging, I think it would be good to add a JMXReporter test which checks that we can start multiple JMXReporter on the same machine and that we can query them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68043825
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -265,4 +329,72 @@ public Object getValue() {
return gauge.getValue();
}
}
+
+ /**
+ * JMX Server implementation that JMX clients can connect to.
+ *
+ * Heavily based on j256 simplejmx project
+ *
+ * https://github.com/j256/simplejmx/blob/master/src/main/java/com/j256/simplejmx/server/JmxServer.java
+ */
+ private static class JMXServer {
+ private int port;
+ private Registry rmiRegistry;
+ private JMXConnectorServer connector;
+
+ public JMXServer(int port) {
+ this.port = port;
+ }
+
+ public void start() throws IOException {
+ startRmiRegistry();
+ startJmxService();
+ }
+
+ public void stop() throws IOException {
+ if (connector != null) {
+ try {
+ connector.stop();
+ } finally {
+ connector = null;
+ }
+ }
+ if (rmiRegistry != null) {
+ try {
+ UnicastRemoteObject.unexportObject(rmiRegistry, true);
+ } catch (NoSuchObjectException e) {
+ throw new IOException("Could not unexport our RMI registry", e);
+ } finally {
+ rmiRegistry = null;
+ }
+ }
+ }
+
+ private void startRmiRegistry() throws IOException {
+ if (rmiRegistry != null) {
+ return;
+ }
+ rmiRegistry = LocateRegistry.createRegistry(port);
+ }
+
+ private void startJmxService() throws IOException {
+ if (connector != null) {
+ return;
+ }
+ String serverHost = "localhost";
+ String registryHost = "";
+ String serviceUrl =
+ "service:jmx:rmi://" + serverHost + ":" + port + "/jndi/rmi://" + registryHost + ":" + port + "/jmxrmi";
--- End diff --
Maybe we could try this out to know for sure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68043573
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
--- End diff --
Have you checked out `NetUtils.getPortRangeFromString` to iterate over a range of ports? This returns an `Iterator` over all defined ports. With that you can also define port ranges like: `1911, 2222-3333, 42-1337`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68209115
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -265,4 +329,72 @@ public Object getValue() {
return gauge.getValue();
}
}
+
+ /**
+ * JMX Server implementation that JMX clients can connect to.
+ *
+ * Heavily based on j256 simplejmx project
+ *
+ * https://github.com/j256/simplejmx/blob/master/src/main/java/com/j256/simplejmx/server/JmxServer.java
+ */
+ private static class JMXServer {
+ private int port;
+ private Registry rmiRegistry;
--- End diff --
dunno. It _looked_ important in the original, so i kept it. I only removed codepaths that wouldn't be used and the resulting unused fields etc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2145
Excellent @zentol :-)
+1 for merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68238504
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
+ int port = Integer.parseInt(ports[0]);
+ jmxServer = new JMXServer(port);
+ try {
+ jmxServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Could not start JMX server on port " + port + ".");
+ }
+ return jmxServer;
+ } else { //port range was configured
+ int start = Integer.parseInt(ports[0]);
+ int end = Integer.parseInt(ports[1]);
+ while (true) {
+ try {
+ jmxServer = new JMXServer(start);
+ jmxServer.start();
+ LOG.info("Starting JMX on port " + start + ".");
--- End diff --
Sure, but it might be interesting to see explicitly which ports were tried, imho.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68042265
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -265,4 +329,72 @@ public Object getValue() {
return gauge.getValue();
}
}
+
+ /**
+ * JMX Server implementation that JMX clients can connect to.
+ *
+ * Heavily based on j256 simplejmx project
+ *
+ * https://github.com/j256/simplejmx/blob/master/src/main/java/com/j256/simplejmx/server/JmxServer.java
+ */
+ private static class JMXServer {
+ private int port;
+ private Registry rmiRegistry;
+ private JMXConnectorServer connector;
+
+ public JMXServer(int port) {
+ this.port = port;
+ }
+
+ public void start() throws IOException {
+ startRmiRegistry();
+ startJmxService();
+ }
+
+ public void stop() throws IOException {
+ if (connector != null) {
+ try {
+ connector.stop();
+ } finally {
+ connector = null;
+ }
+ }
+ if (rmiRegistry != null) {
+ try {
+ UnicastRemoteObject.unexportObject(rmiRegistry, true);
+ } catch (NoSuchObjectException e) {
+ throw new IOException("Could not unexport our RMI registry", e);
+ } finally {
+ rmiRegistry = null;
+ }
+ }
+ }
+
+ private void startRmiRegistry() throws IOException {
+ if (rmiRegistry != null) {
+ return;
+ }
+ rmiRegistry = LocateRegistry.createRegistry(port);
+ }
+
+ private void startJmxService() throws IOException {
+ if (connector != null) {
+ return;
+ }
+ String serverHost = "localhost";
+ String registryHost = "";
+ String serviceUrl =
+ "service:jmx:rmi://" + serverHost + ":" + port + "/jndi/rmi://" + registryHost + ":" + port + "/jmxrmi";
--- End diff --
Can I connect from a remote machine to this JMX server?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68209393
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
+ int port = Integer.parseInt(ports[0]);
+ jmxServer = new JMXServer(port);
+ try {
+ jmxServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Could not start JMX server on port " + port + ".");
+ }
+ return jmxServer;
+ } else { //port range was configured
+ int start = Integer.parseInt(ports[0]);
+ int end = Integer.parseInt(ports[1]);
+ while (true) {
+ try {
+ jmxServer = new JMXServer(start);
+ jmxServer.start();
+ LOG.info("Starting JMX on port " + start + ".");
--- End diff --
I can add it as a debug statement, but can that information not be inferred from the configured ports and the final port used? (all configured ports < final port)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2145
merging
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68396189
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -265,4 +329,72 @@ public Object getValue() {
return gauge.getValue();
}
}
+
+ /**
+ * JMX Server implementation that JMX clients can connect to.
+ *
+ * Heavily based on j256 simplejmx project
+ *
+ * https://github.com/j256/simplejmx/blob/master/src/main/java/com/j256/simplejmx/server/JmxServer.java
+ */
+ private static class JMXServer {
+ private int port;
+ private Registry rmiRegistry;
+ private JMXConnectorServer connector;
+
+ public JMXServer(int port) {
+ this.port = port;
+ }
+
+ public void start() throws IOException {
+ startRmiRegistry();
+ startJmxService();
+ }
+
+ public void stop() throws IOException {
+ if (connector != null) {
+ try {
+ connector.stop();
+ } finally {
+ connector = null;
+ }
+ }
+ if (rmiRegistry != null) {
+ try {
+ UnicastRemoteObject.unexportObject(rmiRegistry, true);
+ } catch (NoSuchObjectException e) {
+ throw new IOException("Could not unexport our RMI registry", e);
+ } finally {
+ rmiRegistry = null;
+ }
+ }
+ }
+
+ private void startRmiRegistry() throws IOException {
+ if (rmiRegistry != null) {
+ return;
+ }
+ rmiRegistry = LocateRegistry.createRegistry(port);
+ }
+
+ private void startJmxService() throws IOException {
+ if (connector != null) {
+ return;
+ }
+ String serverHost = "localhost";
+ String registryHost = "";
+ String serviceUrl =
+ "service:jmx:rmi://" + serverHost + ":" + port + "/jndi/rmi://" + registryHost + ":" + port + "/jmxrmi";
--- End diff --
No, it does not allow connecting via remote. This also applies to the current version.
I also remembered that this was not considered a required feature during implementation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68043291
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
+ int port = Integer.parseInt(ports[0]);
+ jmxServer = new JMXServer(port);
+ try {
+ jmxServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Could not start JMX server on port " + port + ".");
+ }
+ return jmxServer;
+ } else { //port range was configured
+ int start = Integer.parseInt(ports[0]);
+ int end = Integer.parseInt(ports[1]);
+ while (true) {
+ try {
+ jmxServer = new JMXServer(start);
+ jmxServer.start();
+ LOG.info("Starting JMX on port " + start + ".");
--- End diff --
But then it should say "Started" instead of "Starting"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68041287
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
+ int port = Integer.parseInt(ports[0]);
+ jmxServer = new JMXServer(port);
+ try {
+ jmxServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Could not start JMX server on port " + port + ".");
+ }
+ return jmxServer;
+ } else { //port range was configured
+ int start = Integer.parseInt(ports[0]);
+ int end = Integer.parseInt(ports[1]);
+ while (true) {
+ try {
+ jmxServer = new JMXServer(start);
+ jmxServer.start();
+ LOG.info("Starting JMX on port " + start + ".");
--- End diff --
The log statement should appear before the `start` call.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2145
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/2145
@tillrohrmann I've addressed your comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68238384
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -265,4 +329,72 @@ public Object getValue() {
return gauge.getValue();
}
}
+
+ /**
+ * JMX Server implementation that JMX clients can connect to.
+ *
+ * Heavily based on j256 simplejmx project
+ *
+ * https://github.com/j256/simplejmx/blob/master/src/main/java/com/j256/simplejmx/server/JmxServer.java
+ */
+ private static class JMXServer {
+ private int port;
+ private Registry rmiRegistry;
--- End diff --
Hmm, if we don't understand the code and, thus, cannot explain what it does, we shouldn't include the code. The question who should maintain this kind of code. I think it would be best if you could do some research to clarify how the `JMXServer` works and add some comments explaining the different components.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68209148
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
--- End diff --
that would certainly be nicer
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68044577
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -265,4 +329,72 @@ public Object getValue() {
return gauge.getValue();
}
}
+
+ /**
+ * JMX Server implementation that JMX clients can connect to.
+ *
+ * Heavily based on j256 simplejmx project
+ *
+ * https://github.com/j256/simplejmx/blob/master/src/main/java/com/j256/simplejmx/server/JmxServer.java
+ */
+ private static class JMXServer {
+ private int port;
+ private Registry rmiRegistry;
--- End diff --
What does this `rmiRegistry` do?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68041836
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -73,10 +86,61 @@ public JMXReporter() {
// ------------------------------------------------------------------------
@Override
- public void open(Configuration config) {}
+ public void open(Configuration config) {
+ this.jmxServer = startJmxServer(config);
+ }
+
+ private static JMXServer startJmxServer(Configuration config) {
+ JMXServer jmxServer;
+
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, "9010-9025");
+ String[] ports = portRange.split("-");
+
+ if (ports.length == 0 || ports.length > 2) {
+ throw new IllegalArgumentException("JMX port range was configured incorrectly. " +
+ "Expected: <startPort>[-<endPort>] Configured: " + portRange);
+ }
+
+ if (ports.length == 1) { //single port was configured
+ int port = Integer.parseInt(ports[0]);
+ jmxServer = new JMXServer(port);
+ try {
+ jmxServer.start();
+ } catch (IOException e) {
+ throw new RuntimeException("Could not start JMX server on port " + port + ".");
+ }
+ return jmxServer;
+ } else { //port range was configured
+ int start = Integer.parseInt(ports[0]);
+ int end = Integer.parseInt(ports[1]);
+ while (true) {
+ try {
+ jmxServer = new JMXServer(start);
+ jmxServer.start();
+ LOG.info("Starting JMX on port " + start + ".");
--- End diff --
no, it has to appear behind start() since that is the exception-throwing call.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68041209
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/MetricRegistry.java ---
@@ -85,7 +87,14 @@ public MetricRegistry(Configuration config) {
if (className == null) {
// by default, create JMX metrics
LOG.info("No metrics reporter configured, exposing metrics via JMX");
+
+ Configuration reporterConfig = new Configuration();
+ String portRange = config.getString(KEY_METRICS_JMX_PORT, null);
+ if (portRange != null) {
+ reporterConfig.setString(KEY_METRICS_JMX_PORT, portRange);
+ }
this.reporter = new JMXReporter();
+ this.reporter.open(reporterConfig);
--- End diff --
There is also the catch clause of the else branch where we create a `JMXReporter`, but we never call the `open` method there. I think we should add a proper `open` call there as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2145: [FLINK-4087] [metrics] Improved JMX port handling
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2145#discussion_r68043651
--- Diff: flink-core/src/main/java/org/apache/flink/metrics/reporter/JMXReporter.java ---
@@ -265,4 +329,72 @@ public Object getValue() {
return gauge.getValue();
}
}
+
+ /**
+ * JMX Server implementation that JMX clients can connect to.
+ *
+ * Heavily based on j256 simplejmx project
+ *
+ * https://github.com/j256/simplejmx/blob/master/src/main/java/com/j256/simplejmx/server/JmxServer.java
+ */
+ private static class JMXServer {
+ private int port;
+ private Registry rmiRegistry;
+ private JMXConnectorServer connector;
+
+ public JMXServer(int port) {
+ this.port = port;
+ }
+
+ public void start() throws IOException {
+ startRmiRegistry();
+ startJmxService();
+ }
+
+ public void stop() throws IOException {
+ if (connector != null) {
+ try {
+ connector.stop();
+ } finally {
+ connector = null;
+ }
+ }
+ if (rmiRegistry != null) {
+ try {
+ UnicastRemoteObject.unexportObject(rmiRegistry, true);
+ } catch (NoSuchObjectException e) {
+ throw new IOException("Could not unexport our RMI registry", e);
+ } finally {
+ rmiRegistry = null;
+ }
+ }
+ }
+
+ private void startRmiRegistry() throws IOException {
+ if (rmiRegistry != null) {
+ return;
+ }
+ rmiRegistry = LocateRegistry.createRegistry(port);
+ }
+
+ private void startJmxService() throws IOException {
+ if (connector != null) {
+ return;
+ }
+ String serverHost = "localhost";
+ String registryHost = "";
+ String serviceUrl =
+ "service:jmx:rmi://" + serverHost + ":" + port + "/jndi/rmi://" + registryHost + ":" + port + "/jmxrmi";
--- End diff --
i know that you can connect to it on the same machine using the remote protocol. In t hat regard it exhibits the same behaviour as the previous version, as such i would assume that yes, it does allow remote machines.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---