[
https://issues.apache.org/jira/browse/ARTEMIS-3365?focusedWorklogId=616180&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-616180
]
ASF GitHub Bot logged work on ARTEMIS-3365:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 29/Jun/21 13:24
Start Date: 29/Jun/21 13:24
Worklog Time Spent: 10m
Work Description: gemmellr commented on a change in pull request #3634:
URL: https://github.com/apache/activemq-artemis/pull/3634#discussion_r659877781
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
##########
@@ -469,7 +473,45 @@ public void onRemoteOpen(Connection connection) throws
Exception {
} catch (Exception e) {
log.error("Error init connection", e);
}
- if (!validateConnection(connection)) {
+
+ boolean redirected = false;
+ if (connectionCallback.getTransportConnection().getRedirectTo() != null)
{
+ org.apache.activemq.artemis.spi.core.remoting.Connection
transportConnection = connectionCallback.getTransportConnection();
+ Target target = protocolManager.getServer().getBalancerManager()
+ .getBalancer(transportConnection.getRedirectTo())
+ .getTarget(transportConnection, connection.getRemoteContainer(),
handler.getSASLResult().getUser());
+
+ if (target != null) {
+
ActiveMQServerLogger.LOGGER.redirectClientConnection(transportConnection,
target);
+
+ if (!target.isLocal()) {
+ String host =
ConfigurationHelper.getStringProperty(TransportConstants.HOST_PROP_NAME,
TransportConstants.DEFAULT_HOST, target.getConnector().getParams());
+ int port =
ConfigurationHelper.getIntProperty(TransportConstants.PORT_PROP_NAME,
TransportConstants.DEFAULT_PORT, target.getConnector().getParams());
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.REDIRECT);
+ error.setDescription(ConnectionError.REDIRECT.toString());
Review comment:
Just adding the condition name again is a pretty unhelpful description.
Clients will likely print both. Might be nicer to include something in the
description giving context for the reason, aiding users to know whats going on,
like 'Balancer is redirecting...'
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
##########
@@ -469,7 +473,45 @@ public void onRemoteOpen(Connection connection) throws
Exception {
} catch (Exception e) {
log.error("Error init connection", e);
}
- if (!validateConnection(connection)) {
+
+ boolean redirected = false;
+ if (connectionCallback.getTransportConnection().getRedirectTo() != null)
{
+ org.apache.activemq.artemis.spi.core.remoting.Connection
transportConnection = connectionCallback.getTransportConnection();
+ Target target = protocolManager.getServer().getBalancerManager()
+ .getBalancer(transportConnection.getRedirectTo())
+ .getTarget(transportConnection, connection.getRemoteContainer(),
handler.getSASLResult().getUser());
+
+ if (target != null) {
+
ActiveMQServerLogger.LOGGER.redirectClientConnection(transportConnection,
target);
+
+ if (!target.isLocal()) {
+ String host =
ConfigurationHelper.getStringProperty(TransportConstants.HOST_PROP_NAME,
TransportConstants.DEFAULT_HOST, target.getConnector().getParams());
+ int port =
ConfigurationHelper.getIntProperty(TransportConstants.PORT_PROP_NAME,
TransportConstants.DEFAULT_PORT, target.getConnector().getParams());
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.REDIRECT);
+ error.setDescription(ConnectionError.REDIRECT.toString());
+ Map<Symbol, String> info = new HashMap<>();
+ info.put(AmqpSupport.NETWORK_HOST, host);
+ info.put(AmqpSupport.PORT, Integer.toString(port));
+ error.setInfo(info);
+ connection.setCondition(error);
+
+ redirected = true;
+ }
+ } else {
+
ActiveMQServerLogger.LOGGER.cannotRedirectClientConnection(transportConnection);
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.CONNECTION_FORCED);
+ error.setDescription(ConnectionError.CONNECTION_FORCED.toString());
Review comment:
Again, not particularly helpful setting the description as the
condition. Perhaps some context on the problem to help. Why did this happen,
what can poor confused user do about it etc.
What is a 'local redirect' even? EDIT: According to the docs later in the
diff, its a redirect to the broker doing the balancing/redirects, and there
seems to be an explicit toggle to enable this situation to happen. As such its
not clear to be why this is set up as a failure case causing the connection to
be forced closed without explanation. Shouldnt it simply succeed, like any old
connection would without the balancer?
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
##########
@@ -469,7 +473,45 @@ public void onRemoteOpen(Connection connection) throws
Exception {
} catch (Exception e) {
log.error("Error init connection", e);
}
- if (!validateConnection(connection)) {
+
+ boolean redirected = false;
+ if (connectionCallback.getTransportConnection().getRedirectTo() != null)
{
+ org.apache.activemq.artemis.spi.core.remoting.Connection
transportConnection = connectionCallback.getTransportConnection();
+ Target target = protocolManager.getServer().getBalancerManager()
+ .getBalancer(transportConnection.getRedirectTo())
+ .getTarget(transportConnection, connection.getRemoteContainer(),
handler.getSASLResult().getUser());
+
+ if (target != null) {
+
ActiveMQServerLogger.LOGGER.redirectClientConnection(transportConnection,
target);
+
+ if (!target.isLocal()) {
+ String host =
ConfigurationHelper.getStringProperty(TransportConstants.HOST_PROP_NAME,
TransportConstants.DEFAULT_HOST, target.getConnector().getParams());
+ int port =
ConfigurationHelper.getIntProperty(TransportConstants.PORT_PROP_NAME,
TransportConstants.DEFAULT_PORT, target.getConnector().getParams());
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.REDIRECT);
+ error.setDescription(ConnectionError.REDIRECT.toString());
+ Map<Symbol, String> info = new HashMap<>();
+ info.put(AmqpSupport.NETWORK_HOST, host);
+ info.put(AmqpSupport.PORT, Integer.toString(port));
Review comment:
Although the spec amusingly does not specify a specific type, I would
probably default to sending a numeric type for 'port'. I'd just go with integer
personally (or UnsignedInteger after that, but I tend to avoid the unsigned
types personally)
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
##########
@@ -469,7 +473,45 @@ public void onRemoteOpen(Connection connection) throws
Exception {
} catch (Exception e) {
log.error("Error init connection", e);
}
- if (!validateConnection(connection)) {
+
+ boolean redirected = false;
+ if (connectionCallback.getTransportConnection().getRedirectTo() != null)
{
+ org.apache.activemq.artemis.spi.core.remoting.Connection
transportConnection = connectionCallback.getTransportConnection();
+ Target target = protocolManager.getServer().getBalancerManager()
+ .getBalancer(transportConnection.getRedirectTo())
+ .getTarget(transportConnection, connection.getRemoteContainer(),
handler.getSASLResult().getUser());
+
+ if (target != null) {
+
ActiveMQServerLogger.LOGGER.redirectClientConnection(transportConnection,
target);
+
+ if (!target.isLocal()) {
+ String host =
ConfigurationHelper.getStringProperty(TransportConstants.HOST_PROP_NAME,
TransportConstants.DEFAULT_HOST, target.getConnector().getParams());
+ int port =
ConfigurationHelper.getIntProperty(TransportConstants.PORT_PROP_NAME,
TransportConstants.DEFAULT_PORT, target.getConnector().getParams());
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.REDIRECT);
+ error.setDescription(ConnectionError.REDIRECT.toString());
+ Map<Symbol, String> info = new HashMap<>();
+ info.put(AmqpSupport.NETWORK_HOST, host);
+ info.put(AmqpSupport.PORT, Integer.toString(port));
+ error.setInfo(info);
+ connection.setCondition(error);
+
+ redirected = true;
+ }
+ } else {
+
ActiveMQServerLogger.LOGGER.cannotRedirectClientConnection(transportConnection);
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.CONNECTION_FORCED);
+ error.setDescription(ConnectionError.CONNECTION_FORCED.toString());
Review comment:
Again, not particularly helpful setting the condition as the
description. Perhaps some context on the problem to help. Why did this happen,
what can poor confused user do about it etc.
What is a 'local redirect' even? EDIT: According to the docs later in the
diff, its a redirect to the broker doing the balancing/redirects, and there
seems to be an explicit toggle to enable this situation to happen. As such its
not clear to be why this is set up as a failure case causing the connection to
be forced closed without explanation. Shouldnt it simply succeed, like any old
connection would without the balancer?
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
##########
@@ -469,7 +473,45 @@ public void onRemoteOpen(Connection connection) throws
Exception {
} catch (Exception e) {
log.error("Error init connection", e);
}
- if (!validateConnection(connection)) {
+
+ boolean redirected = false;
+ if (connectionCallback.getTransportConnection().getRedirectTo() != null)
{
+ org.apache.activemq.artemis.spi.core.remoting.Connection
transportConnection = connectionCallback.getTransportConnection();
+ Target target = protocolManager.getServer().getBalancerManager()
+ .getBalancer(transportConnection.getRedirectTo())
+ .getTarget(transportConnection, connection.getRemoteContainer(),
handler.getSASLResult().getUser());
+
+ if (target != null) {
+
ActiveMQServerLogger.LOGGER.redirectClientConnection(transportConnection,
target);
+
+ if (!target.isLocal()) {
+ String host =
ConfigurationHelper.getStringProperty(TransportConstants.HOST_PROP_NAME,
TransportConstants.DEFAULT_HOST, target.getConnector().getParams());
+ int port =
ConfigurationHelper.getIntProperty(TransportConstants.PORT_PROP_NAME,
TransportConstants.DEFAULT_PORT, target.getConnector().getParams());
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.REDIRECT);
+ error.setDescription(ConnectionError.REDIRECT.toString());
+ Map<Symbol, String> info = new HashMap<>();
+ info.put(AmqpSupport.NETWORK_HOST, host);
+ info.put(AmqpSupport.PORT, Integer.toString(port));
+ error.setInfo(info);
+ connection.setCondition(error);
+
+ redirected = true;
+ }
+ } else {
+
ActiveMQServerLogger.LOGGER.cannotRedirectClientConnection(transportConnection);
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.CONNECTION_FORCED);
+ error.setDescription(ConnectionError.CONNECTION_FORCED.toString());
Review comment:
Again, not particularly helpful setting the condition as the
description. Perhaps some context on the problem to help. Why did this happen,
what can poor confused user do about it etc.
What is a 'local redirect' even? EDIT: According to the docs later in the
diff, its a redirect to the broker doing the balancing/redirects, and there
seems to be an explicit toggle to enable this situation to happen. As such its
not clear to me why this is set up as a failure case causing the connection to
be forced closed without explanation. Shouldnt it simply succeed, like any old
connection would without the balancer?
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
##########
@@ -469,7 +473,45 @@ public void onRemoteOpen(Connection connection) throws
Exception {
} catch (Exception e) {
log.error("Error init connection", e);
}
- if (!validateConnection(connection)) {
+
+ boolean redirected = false;
+ if (connectionCallback.getTransportConnection().getRedirectTo() != null)
{
+ org.apache.activemq.artemis.spi.core.remoting.Connection
transportConnection = connectionCallback.getTransportConnection();
+ Target target = protocolManager.getServer().getBalancerManager()
+ .getBalancer(transportConnection.getRedirectTo())
+ .getTarget(transportConnection, connection.getRemoteContainer(),
handler.getSASLResult().getUser());
+
+ if (target != null) {
+
ActiveMQServerLogger.LOGGER.redirectClientConnection(transportConnection,
target);
+
+ if (!target.isLocal()) {
+ String host =
ConfigurationHelper.getStringProperty(TransportConstants.HOST_PROP_NAME,
TransportConstants.DEFAULT_HOST, target.getConnector().getParams());
+ int port =
ConfigurationHelper.getIntProperty(TransportConstants.PORT_PROP_NAME,
TransportConstants.DEFAULT_PORT, target.getConnector().getParams());
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.REDIRECT);
+ error.setDescription(ConnectionError.REDIRECT.toString());
+ Map<Symbol, String> info = new HashMap<>();
+ info.put(AmqpSupport.NETWORK_HOST, host);
+ info.put(AmqpSupport.PORT, Integer.toString(port));
+ error.setInfo(info);
+ connection.setCondition(error);
+
+ redirected = true;
+ }
+ } else {
+
ActiveMQServerLogger.LOGGER.cannotRedirectClientConnection(transportConnection);
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.CONNECTION_FORCED);
+ error.setDescription(ConnectionError.CONNECTION_FORCED.toString());
Review comment:
Oops misread diff, that else bit is when target==null. General sentiment
of 'give a more useful error description' still applies, i.e give some detail
so poor user can perhaps understand and maybe help themselves if they did
something wrong. As is a client is likely to just retry, possibly in a loop if
this case repeats, with no real indication whats going on.
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
##########
@@ -469,7 +474,46 @@ public void onRemoteOpen(Connection connection) throws
Exception {
} catch (Exception e) {
log.error("Error init connection", e);
}
- if (!validateConnection(connection)) {
+
+ boolean redirected = false;
+ if (connectionCallback.getTransportConnection().getRedirectTo() != null)
{
+ org.apache.activemq.artemis.spi.core.remoting.Connection
transportConnection = connectionCallback.getTransportConnection();
+ BrokerBalancer brokerBalancer =
protocolManager.getServer().getBalancerManager()
+ .getBalancer(transportConnection.getRedirectTo());
+ Target target = brokerBalancer.getTarget(transportConnection,
+ connection.getRemoteContainer(),
handler.getSASLResult().getUser());
+
+ if (target != null) {
+
ActiveMQServerLogger.LOGGER.redirectClientConnection(transportConnection,
target);
+
+ if (!target.isLocal()) {
+ String host =
ConfigurationHelper.getStringProperty(TransportConstants.HOST_PROP_NAME,
TransportConstants.DEFAULT_HOST, target.getConnector().getParams());
+ int port =
ConfigurationHelper.getIntProperty(TransportConstants.PORT_PROP_NAME,
TransportConstants.DEFAULT_PORT, target.getConnector().getParams());
+
+ ErrorCondition error = new ErrorCondition();
+ error.setCondition(ConnectionError.REDIRECT);
+ error.setDescription(String.format("Connection redirected to %s
by broker balancer %s", brokerBalancer.getName(), target));
Review comment:
Are the args possibly reversed? Havent tried it, but the format string
seems to read more like the balancer name is expected to be second/last?
Either way, the toString() on Target seems like this might result in
including a bunch of config/names some might consider 'internal' to the broker
setup into the description. Or perhaps just not entirely relevant to an AMQP
client/user that doesnt use them.
It might be simpler and clearer just to use the target host:port, which are
already available here.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 616180)
Time Spent: 4h 20m (was: 4h 10m)
> Broker Balancers
> ----------------
>
> Key: ARTEMIS-3365
> URL: https://issues.apache.org/jira/browse/ARTEMIS-3365
> Project: ActiveMQ Artemis
> Issue Type: New Feature
> Reporter: Domenico Francesco Bruscino
> Assignee: Domenico Francesco Bruscino
> Priority: Major
> Time Spent: 4h 20m
> Remaining Estimate: 0h
>
> This feature adds the broker balancers to distribute the incoming client
> connections across multiple brokers.
> It provides a native redirection for supported clients and a new management
> API for other clients. The native redirection can be enabled per acceptor and
> is supported only for CORE and AMQP clients.
> See the [draft
> documentation|https://github.com/brusdev/activemq-artemis/blob/broker_balancers/docs/user-manual/en/broker-balancers.md]
> for further details.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
