You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/01/19 14:28:22 UTC
[GitHub] [activemq-artemis] TomasHofman opened a new pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
TomasHofman opened a new pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412
…which accepts the callTimeout attribute
https://issues.apache.org/jira/browse/ARTEMIS-3074
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman edited a comment on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman edited a comment on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-767069329
Sorry for the long delays, I'm in the middle of a release...
I realized there are further complications regarding default values. E.g. user and password fields default to `"ACTIVEMQ.CLUSTER.ADMIN.USER"` and `"CHANGE ME!!"` and we need to be able to set them to `null`, which means null values have to be deserialized. I chose following behavior:
* All non-null values are always serialized to JSON, even if they are default (this could be changed to serialize only if value is non-default, but to me that seems it would be confusing),
* if JSON string doesn't contain a field, during deserialization it will be instantiated with its' default value,
* if JSON string contains a field with `null`, it will be instantiated to null, regardless of it's default value (unless it's a primitive type field which would fail).
I added some more tests to BridgeConfigurationTest to verify above behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman edited a comment on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman edited a comment on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-767069329
Sorry for the long delays, I'm in the middle of a release...
I realized there are further complications regarding default values. E.g. user and password fields default to `"ACTIVEMQ.CLUSTER.ADMIN.USER"` and `"CHANGE ME!!"` and we need to be able to set them to `null`, which means null values have to be deserialized. I chose following behavior:
* All non-null values are always serialized to JSON, even if they are default (this could be changed to serialize only if value is non-default, but to me that seems it would be confusing),
* if JSON string doesn't contain a field, during deserialization it will be instantiated with its' default value,
* if JSON string contains a field with `null`, it will be instantiated to null, regardless of it's default value (unless it's a primitive type field which would fail).
I added some more tests to BridgeConfigurationTest to verify above behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r562318018
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3822,6 +3822,30 @@ public void createBridge(final String name,
}
}
+ @Override
+ public String createBridge(String bridgeConfigurationAsJson) throws Exception {
Review comment:
This can return `void`. The only reason the `createQueue` JSON method returns a `String` is because the broker applies address settings to the queue on the fly to end up at the final configuration and that's something the client might like to know. In this case the broker just takes what it's given.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/config/BridgeConfiguration.java
##########
@@ -360,6 +498,98 @@ public BridgeConfiguration setCallTimeout(long callTimeout) {
return this;
}
+ /**
+ * This method returns a JSON-formatted {@code String} representation of this {@code QueueConfiguration}. It is a
+ * simple collection of key/value pairs. The keys used are referenced in {@link #set(String, String)}.
+ *
+ * @return a JSON-formatted {@code String} representation of this {@code QueueConfiguration}
Review comment:
The JavaDoc references `QueueConfiguration`.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/config/BridgeConfiguration.java
##########
@@ -360,6 +498,98 @@ public BridgeConfiguration setCallTimeout(long callTimeout) {
return this;
}
+ /**
+ * This method returns a JSON-formatted {@code String} representation of this {@code QueueConfiguration}. It is a
+ * simple collection of key/value pairs. The keys used are referenced in {@link #set(String, String)}.
+ *
+ * @return a JSON-formatted {@code String} representation of this {@code QueueConfiguration}
+ */
+ public String toJSON() {
+ JsonObjectBuilder builder = JsonLoader.createObjectBuilder();
+
+ if (getName() != null) {
+ builder.add(NAME, getName());
+ }
+ if (getQueueName() != null) {
+ builder.add(QUEUE_NAME, getQueueName());
+ }
+ if (getForwardingAddress() != null) {
+ builder.add(FORWARDING_ADDRESS, getForwardingAddress());
+ }
+ if (getFilterString() != null) {
+ builder.add(FILTER_STRING, getFilterString());
+ }
+ if (getStaticConnectors() != null) {
+ JsonArrayBuilder arrayBuilder = JsonLoader.createArrayBuilder();
+ getStaticConnectors().forEach(arrayBuilder::add);
+ builder.add(STATIC_CONNECTORS, arrayBuilder);
+ }
+ if (getDiscoveryGroupName() != null) {
+ builder.add(DISCOVERY_GROUP_NAME, getDiscoveryGroupName());
+ }
+ builder.add(HA, isHA());
+ if (getTransformerConfiguration() != null) {
+ TransformerConfiguration tc = getTransformerConfiguration();
+ JsonObjectBuilder tcBuilder = JsonLoader.createObjectBuilder()
+ .add(TransformerConfiguration.CLASS_NAME, tc.getClassName());
+ if (tc.getProperties() != null && tc.getProperties().size() > 0) {
+ JsonObjectBuilder propBuilder = JsonLoader.createObjectBuilder();
+ tc.getProperties().forEach(propBuilder::add);
+ tcBuilder.add(TransformerConfiguration.PROPERTIES, propBuilder);
+ }
+ builder.add(TRANSFORMER_CONFIGURATION, tcBuilder);
+ }
+ builder.add(RETRY_INTERVAL, getRetryInterval());
+ builder.add(RETRY_INTERVAL_MULTIPLIER, getRetryIntervalMultiplier());
+ builder.add(INITIAL_CONNECT_ATTEMPTS, getInitialConnectAttempts());
+ builder.add(RECONNECT_ATTEMPTS, getReconnectAttempts());
+ builder.add(RECONNECT_ATTEMPTS_ON_SAME_NODE, getReconnectAttemptsOnSameNode());
+ builder.add(USE_DUPLICATE_DETECTION, isUseDuplicateDetection());
+ builder.add(CONFIRMATION_WINDOW_SIZE, getConfirmationWindowSize());
+ builder.add(PRODUCER_WINDOW_SIZE, getProducerWindowSize());
+ builder.add(CLIENT_FAILURE_CHECK_PERIOD, getClientFailureCheckPeriod());
+ if (getUser() != null) {
+ builder.add(USER, getUser());
+ }
+ if (getPassword() != null) {
+ builder.add(PASSWORD, getPassword());
+ }
+ builder.add(CONNECTION_TTL, getConnectionTTL());
+ builder.add(MAX_RETRY_INTERVAL, getMaxRetryInterval());
+ builder.add(MIN_LARGE_MESSAGE_SIZE, getMinLargeMessageSize());
+ builder.add(CALL_TIMEOUT, getCallTimeout());
+ builder.add(ROUTING_TYPE, getRoutingType().name());
+
+ return builder.build().toString();
+ }
+
+ /**
+ * This method returns a {@code QueueConfiguration} created from the JSON-formatted input {@code String}. The input
Review comment:
The JavaDoc references `QueueConfiguration`.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3822,6 +3822,30 @@ public void createBridge(final String name,
}
}
+ @Override
+ public String createBridge(String bridgeConfigurationAsJson) throws Exception {
+ if (AuditLogger.isEnabled()) {
+ AuditLogger.createBridge(this.server, bridgeConfigurationAsJson);
+ }
+ checkStarted();
+
+ clearIO();
+
+ try {
+ // when the QueueConfiguration is passed through createQueue all of its defaults get set which we return to the caller
Review comment:
The comment references `QueueConfiguration`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-765062691
Aside from my inline comments I've got a few others...
- Please deprecate all the other `createBridge` management methods.
- Add another test like `org.apache.activemq.artemis.tests.integration.management.ActiveMQServerControlTest#testCreateAndDestroyBridge` using the new method.
> ...it would allow also us to create method `ActiveMQServerControl#createBridge(BridgeConfiguration)` :), which may be nice type safe addition to the JSON string version.
Management methods should only take simple types so they can be invoked from JMX tools as well as HTTP via Jolokia. The only time a complex object (e.g. `BridgeConfiguration`) would be used is if the invoking client was in the same JVM but in that case they could use the embedded broker's configuration API directly and won't need the management method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-763614891
@jbertram I implemented the `createBridge(jsonString)` method, can you check if it is what you imagined?
I didn't move the `BridgeConfiguration` class into `activemq-commons` module as it wasn't necessary in the scope of work I did. It rather looked problematic as I would have to move other classes with it (`ActiveMQDefaultConfiguration`, `ActiveMQClient`, `ComponentConfigurationRoutingType`) which `BridgeConfiguration` depends on.
If we were to move it out of `activemq-server` module, would the `artemis-core-client` module be a suitable place? This seems easier judging from how the classes are coupled, and it would allow also us to create method `ActiveMQServerControl#createBridge(BridgeConfiguration)` :), which may be nice type safe addition to the JSON string version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-768513610
The PR looks good and I'm ready to merge, but I had one additional question...
As I was reviewing the PR I noticed that while the bridge configuration references a connector there isn't actually any way to add a new connector via the management API which means every new bridge will have to use a connector which *already exists*. This is a gap in functionality that has been there for a long time, but I just now noticed it. Is this gap going to impact your use-case? If not, I'll go ahead and merge and then send a PR later for a new management method for adding a connector.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r560944089
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3796,19 +3796,58 @@ public void createBridge(final String name,
final boolean ha,
final String user,
final String password) throws Exception {
+ createBridge(name,
+ queueName,
+ forwardingAddress,
+ filterString,
+ transformerClassName,
+ retryInterval,
+ retryIntervalMultiplier,
+ initialConnectAttempts,
+ reconnectAttempts,
+ useDuplicateDetection,
+ confirmationWindowSize,
+ clientFailureCheckPeriod,
+ staticConnectorsOrDiscoveryGroup,
+ useDiscoveryGroup,
+ ha,
+ user,
+ password,
+ ActiveMQClient.DEFAULT_CALL_TIMEOUT);
+ }
+
+ @Override
+ public void createBridge(final String name,
+ final String queueName,
+ final String forwardingAddress,
+ final String filterString,
+ final String transformerClassName,
+ final long retryInterval,
+ final double retryIntervalMultiplier,
+ final int initialConnectAttempts,
+ final int reconnectAttempts,
+ final boolean useDuplicateDetection,
+ final int confirmationWindowSize,
+ final long clientFailureCheckPeriod,
+ final String staticConnectorsOrDiscoveryGroup,
+ boolean useDiscoveryGroup,
+ final boolean ha,
+ final String user,
+ final String password,
+ final long callTimeout) throws Exception {
if (AuditLogger.isEnabled()) {
AuditLogger.createBridge(this.server, name, queueName, forwardingAddress, filterString,
transformerClassName, retryInterval, retryIntervalMultiplier, initialConnectAttempts,
reconnectAttempts, useDuplicateDetection, confirmationWindowSize, clientFailureCheckPeriod,
- staticConnectorsOrDiscoveryGroup, useDiscoveryGroup, ha, user, "****");
+ staticConnectorsOrDiscoveryGroup, useDiscoveryGroup, ha, user, "****", callTimeout);
}
checkStarted();
clearIO();
try {
TransformerConfiguration transformerConfiguration = transformerClassName == null ? null : new TransformerConfiguration(transformerClassName);
Review comment:
Fixed in the master branch in the mean time. :+1:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-763049905
@jbertram no problem, I will make the change as you suggested.
Thanks for all the input!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] brusdev commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
brusdev commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r560235363
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3796,19 +3796,58 @@ public void createBridge(final String name,
final boolean ha,
final String user,
final String password) throws Exception {
+ createBridge(name,
+ queueName,
+ forwardingAddress,
+ filterString,
+ transformerClassName,
+ retryInterval,
+ retryIntervalMultiplier,
+ initialConnectAttempts,
+ reconnectAttempts,
+ useDuplicateDetection,
+ confirmationWindowSize,
+ clientFailureCheckPeriod,
+ staticConnectorsOrDiscoveryGroup,
+ useDiscoveryGroup,
+ ha,
+ user,
+ password,
+ ActiveMQClient.DEFAULT_CALL_TIMEOUT);
+ }
+
+ @Override
+ public void createBridge(final String name,
+ final String queueName,
+ final String forwardingAddress,
+ final String filterString,
+ final String transformerClassName,
+ final long retryInterval,
+ final double retryIntervalMultiplier,
+ final int initialConnectAttempts,
+ final int reconnectAttempts,
+ final boolean useDuplicateDetection,
+ final int confirmationWindowSize,
+ final long clientFailureCheckPeriod,
+ final String staticConnectorsOrDiscoveryGroup,
+ boolean useDiscoveryGroup,
+ final boolean ha,
+ final String user,
+ final String password,
+ final long callTimeout) throws Exception {
if (AuditLogger.isEnabled()) {
AuditLogger.createBridge(this.server, name, queueName, forwardingAddress, filterString,
transformerClassName, retryInterval, retryIntervalMultiplier, initialConnectAttempts,
reconnectAttempts, useDuplicateDetection, confirmationWindowSize, clientFailureCheckPeriod,
- staticConnectorsOrDiscoveryGroup, useDiscoveryGroup, ha, user, "****");
+ staticConnectorsOrDiscoveryGroup, useDiscoveryGroup, ha, user, "****", callTimeout);
}
checkStarted();
clearIO();
try {
TransformerConfiguration transformerConfiguration = transformerClassName == null ? null : new TransformerConfiguration(transformerClassName);
Review comment:
You should check if `transformerClassName` isEmpty to work properly with hawtio2, see ARTEMIS-3072 / #3410 for further details.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r560240289
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3796,19 +3796,58 @@ public void createBridge(final String name,
final boolean ha,
final String user,
final String password) throws Exception {
+ createBridge(name,
+ queueName,
+ forwardingAddress,
+ filterString,
+ transformerClassName,
+ retryInterval,
+ retryIntervalMultiplier,
+ initialConnectAttempts,
+ reconnectAttempts,
+ useDuplicateDetection,
+ confirmationWindowSize,
+ clientFailureCheckPeriod,
+ staticConnectorsOrDiscoveryGroup,
+ useDiscoveryGroup,
+ ha,
+ user,
+ password,
+ ActiveMQClient.DEFAULT_CALL_TIMEOUT);
+ }
+
+ @Override
+ public void createBridge(final String name,
+ final String queueName,
+ final String forwardingAddress,
+ final String filterString,
+ final String transformerClassName,
+ final long retryInterval,
+ final double retryIntervalMultiplier,
+ final int initialConnectAttempts,
+ final int reconnectAttempts,
+ final boolean useDuplicateDetection,
+ final int confirmationWindowSize,
+ final long clientFailureCheckPeriod,
+ final String staticConnectorsOrDiscoveryGroup,
+ boolean useDiscoveryGroup,
+ final boolean ha,
+ final String user,
+ final String password,
+ final long callTimeout) throws Exception {
Review comment:
Make this Long, then you can use check on null if to set in config or not. shouldnt be defaulting it at this layer, thats for lower downs to handle, else in future if we change how config item works, we will forget all about this stuff.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r562318018
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3822,6 +3822,30 @@ public void createBridge(final String name,
}
}
+ @Override
+ public String createBridge(String bridgeConfigurationAsJson) throws Exception {
Review comment:
This can return `void`. The only reason the `createQueue` JSON method returns a `String` is because the broker applies address settings to the queue on the fly to end up at the final configuration and that's something the client might like to know. In this case the broker just takes what it's given.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] rvais commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
rvais commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-773280008
I reviewed this PR as well. The tests which are included should be should be sufficient for the purpose. Feel free to reopen this for future Artemis version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-762983709
If you would prefer not to do the change I suggested I could probably send a PR relatively soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-768919366
Hi @jbertram, connectors are not important to me, I only need a way to set bridge call-timeout :). Thank you for your reviews!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-762978719
Given the proliferation of overloaded management methods it was determined awhile back that we'd move to a more de-coupled approach using flexible JSON-based input. See, for example, the method `org.apache.activemq.artemis.api.core.management.ActiveMQServerControl#createQueue(java.lang.String)` which replaces over 10 overloaded variants. The JSON input `String` for this method can be created manually simply based on the parameter names or generated from the `org.apache.activemq.artemis.api.core.QueueConfiguration` object using the `toJSON` method. I recommend you implement a similar method for `createBridge`. The class `org.apache.activemq.artemis.core.config.BridgeConfiguration` already exists and you can use that for these changes (although you'll likely need to move it out of the `activemq-server` module and into the `activemq-commons` module).
I realize the change I'm describing is much larger than the one you already submitted in this PR, but the new method in this PR is just going to be deprecated in a bit so you'll have to change your integration code anyway. I think it's probably best to do it "right" the first time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r560239547
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3796,19 +3796,58 @@ public void createBridge(final String name,
final boolean ha,
final String user,
final String password) throws Exception {
+ createBridge(name,
+ queueName,
+ forwardingAddress,
+ filterString,
+ transformerClassName,
+ retryInterval,
+ retryIntervalMultiplier,
+ initialConnectAttempts,
+ reconnectAttempts,
+ useDuplicateDetection,
+ confirmationWindowSize,
+ clientFailureCheckPeriod,
+ staticConnectorsOrDiscoveryGroup,
+ useDiscoveryGroup,
+ ha,
+ user,
+ password,
+ ActiveMQClient.DEFAULT_CALL_TIMEOUT);
Review comment:
I would suggest this isnt defaulted here.
Should keep original behaviour of not setting it in the configuration, and in new method setting it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r563999139
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3822,6 +3822,30 @@ public void createBridge(final String name,
}
}
+ @Override
+ public String createBridge(String bridgeConfigurationAsJson) throws Exception {
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-765062691
Aside from my inline comments I've got a few others...
- Please deprecate all the other `createBridge` management methods.
- Add another test like `org.apache.activemq.artemis.tests.integration.management.ActiveMQServerControlTest#testCreateAndDestroyBridge` using the new method.
> ...it would allow also us to create method `ActiveMQServerControl#createBridge(BridgeConfiguration)` :), which may be nice type safe addition to the JSON string version.
Management methods should only take simple types so they can be invoked from JMX tools as well as HTTP via Jolokia. The only time a complex object (e.g. `BridgeConfiguration`) would be used is if the invoking client was in the same JVM but in that case they could use the embedded broker's configuration API directly and won't need the management method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r563999139
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3822,6 +3822,30 @@ public void createBridge(final String name,
}
}
+ @Override
+ public String createBridge(String bridgeConfigurationAsJson) throws Exception {
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r562318157
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/config/BridgeConfiguration.java
##########
@@ -360,6 +498,98 @@ public BridgeConfiguration setCallTimeout(long callTimeout) {
return this;
}
+ /**
+ * This method returns a JSON-formatted {@code String} representation of this {@code QueueConfiguration}. It is a
+ * simple collection of key/value pairs. The keys used are referenced in {@link #set(String, String)}.
+ *
+ * @return a JSON-formatted {@code String} representation of this {@code QueueConfiguration}
Review comment:
The JavaDoc references `QueueConfiguration`.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/config/BridgeConfiguration.java
##########
@@ -360,6 +498,98 @@ public BridgeConfiguration setCallTimeout(long callTimeout) {
return this;
}
+ /**
+ * This method returns a JSON-formatted {@code String} representation of this {@code QueueConfiguration}. It is a
+ * simple collection of key/value pairs. The keys used are referenced in {@link #set(String, String)}.
+ *
+ * @return a JSON-formatted {@code String} representation of this {@code QueueConfiguration}
+ */
+ public String toJSON() {
+ JsonObjectBuilder builder = JsonLoader.createObjectBuilder();
+
+ if (getName() != null) {
+ builder.add(NAME, getName());
+ }
+ if (getQueueName() != null) {
+ builder.add(QUEUE_NAME, getQueueName());
+ }
+ if (getForwardingAddress() != null) {
+ builder.add(FORWARDING_ADDRESS, getForwardingAddress());
+ }
+ if (getFilterString() != null) {
+ builder.add(FILTER_STRING, getFilterString());
+ }
+ if (getStaticConnectors() != null) {
+ JsonArrayBuilder arrayBuilder = JsonLoader.createArrayBuilder();
+ getStaticConnectors().forEach(arrayBuilder::add);
+ builder.add(STATIC_CONNECTORS, arrayBuilder);
+ }
+ if (getDiscoveryGroupName() != null) {
+ builder.add(DISCOVERY_GROUP_NAME, getDiscoveryGroupName());
+ }
+ builder.add(HA, isHA());
+ if (getTransformerConfiguration() != null) {
+ TransformerConfiguration tc = getTransformerConfiguration();
+ JsonObjectBuilder tcBuilder = JsonLoader.createObjectBuilder()
+ .add(TransformerConfiguration.CLASS_NAME, tc.getClassName());
+ if (tc.getProperties() != null && tc.getProperties().size() > 0) {
+ JsonObjectBuilder propBuilder = JsonLoader.createObjectBuilder();
+ tc.getProperties().forEach(propBuilder::add);
+ tcBuilder.add(TransformerConfiguration.PROPERTIES, propBuilder);
+ }
+ builder.add(TRANSFORMER_CONFIGURATION, tcBuilder);
+ }
+ builder.add(RETRY_INTERVAL, getRetryInterval());
+ builder.add(RETRY_INTERVAL_MULTIPLIER, getRetryIntervalMultiplier());
+ builder.add(INITIAL_CONNECT_ATTEMPTS, getInitialConnectAttempts());
+ builder.add(RECONNECT_ATTEMPTS, getReconnectAttempts());
+ builder.add(RECONNECT_ATTEMPTS_ON_SAME_NODE, getReconnectAttemptsOnSameNode());
+ builder.add(USE_DUPLICATE_DETECTION, isUseDuplicateDetection());
+ builder.add(CONFIRMATION_WINDOW_SIZE, getConfirmationWindowSize());
+ builder.add(PRODUCER_WINDOW_SIZE, getProducerWindowSize());
+ builder.add(CLIENT_FAILURE_CHECK_PERIOD, getClientFailureCheckPeriod());
+ if (getUser() != null) {
+ builder.add(USER, getUser());
+ }
+ if (getPassword() != null) {
+ builder.add(PASSWORD, getPassword());
+ }
+ builder.add(CONNECTION_TTL, getConnectionTTL());
+ builder.add(MAX_RETRY_INTERVAL, getMaxRetryInterval());
+ builder.add(MIN_LARGE_MESSAGE_SIZE, getMinLargeMessageSize());
+ builder.add(CALL_TIMEOUT, getCallTimeout());
+ builder.add(ROUTING_TYPE, getRoutingType().name());
+
+ return builder.build().toString();
+ }
+
+ /**
+ * This method returns a {@code QueueConfiguration} created from the JSON-formatted input {@code String}. The input
Review comment:
The JavaDoc references `QueueConfiguration`.
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3822,6 +3822,30 @@ public void createBridge(final String name,
}
}
+ @Override
+ public String createBridge(String bridgeConfigurationAsJson) throws Exception {
+ if (AuditLogger.isEnabled()) {
+ AuditLogger.createBridge(this.server, bridgeConfigurationAsJson);
+ }
+ checkStarted();
+
+ clearIO();
+
+ try {
+ // when the QueueConfiguration is passed through createQueue all of its defaults get set which we return to the caller
Review comment:
The comment references `QueueConfiguration`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-767069329
Sorry for the long delays, I'm in the middle of a release...
I realized there are further complications regarding default values. E.g. user and password fields default to `"ACTIVEMQ.CLUSTER.ADMIN.USER"` and `"CHANGE ME!!"` and we need to be able to set them to `null`, which means null values have to be deserialized. I chose following behavior:
* All non-null values are always serialized to JSON, even if they are default (this could be changed to serialize only if value is non-default, but to me that seems it would be confusing),
* if JSON string doesn't contain a field, during deserialization it will be instantiated with its' default value,
* if JSON string contains a field with `null`, it will be instantiated to null, regardless of it's default value (unless it's a primitive type field which would fail).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-767069329
Sorry for the long delays, I'm in the middle of a release...
I realized there are further complications regarding default values. E.g. user and password fields default to `"ACTIVEMQ.CLUSTER.ADMIN.USER"` and `"CHANGE ME!!"` and we need to be able to set them to `null`, which means null values have to be deserialized. I chose following behavior:
* All non-null values are always serialized to JSON, even if they are default (this could be changed to serialize only if value is non-default, but to me that seems it would be confusing),
* if JSON string doesn't contain a field, during deserialization it will be instantiated with its' default value,
* if JSON string contains a field with `null`, it will be instantiated to null, regardless of it's default value (unless it's a primitive type field which would fail).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] rvais commented on pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
rvais commented on pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#issuecomment-773280008
I reviewed this PR as well. The tests which are included should be should be sufficient for the purpose. Feel free to reopen this for future Artemis version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] TomasHofman commented on a change in pull request #3412: ARTEMIS-3074 Add ActiveMQServerControl#createBridge() method variant …
Posted by GitBox <gi...@apache.org>.
TomasHofman commented on a change in pull request #3412:
URL: https://github.com/apache/activemq-artemis/pull/3412#discussion_r560944580
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
##########
@@ -3796,19 +3796,58 @@ public void createBridge(final String name,
final boolean ha,
final String user,
final String password) throws Exception {
+ createBridge(name,
+ queueName,
+ forwardingAddress,
+ filterString,
+ transformerClassName,
+ retryInterval,
+ retryIntervalMultiplier,
+ initialConnectAttempts,
+ reconnectAttempts,
+ useDuplicateDetection,
+ confirmationWindowSize,
+ clientFailureCheckPeriod,
+ staticConnectorsOrDiscoveryGroup,
+ useDiscoveryGroup,
+ ha,
+ user,
+ password,
+ ActiveMQClient.DEFAULT_CALL_TIMEOUT);
Review comment:
Resolved, thanks for input!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org