alex-rufous commented on a change in pull request #98:
URL: https://github.com/apache/qpid-broker-j/pull/98#discussion_r663536930
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/model/port/AmqpPort.java
##########
@@ -56,6 +56,10 @@
String PORT_AMQP_NUMBER_OF_SELECTORS =
"qpid.port.amqp.threadPool.numberOfSelectors";
String PORT_AMQP_ACCEPT_BACKLOG = "qpid.port.amqp.acceptBacklog";
+ String PORT_DETECT_CONNECTION_LOOPING =
"qpid.port.amqp.detect.connection.looping";
+ String PORT_CONNECTION_LOOPING_DETECTION_THRESHOLD =
"qpid.port.amqp.detect.connection.looping.detection.threshold";
Review comment:
As per comment above, the context variable name does not indicate that
it is only for TLS and "unwritten naming convention" is not followed...
What about changing the name to
`qpid.port.amqp.diagnosisOfSslEngineLoopingWarnThreshold` ?
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/model/port/AmqpPort.java
##########
@@ -56,6 +56,10 @@
String PORT_AMQP_NUMBER_OF_SELECTORS =
"qpid.port.amqp.threadPool.numberOfSelectors";
String PORT_AMQP_ACCEPT_BACKLOG = "qpid.port.amqp.acceptBacklog";
+ String PORT_DETECT_CONNECTION_LOOPING =
"qpid.port.amqp.detect.connection.looping";
+ String PORT_CONNECTION_LOOPING_DETECTION_THRESHOLD =
"qpid.port.amqp.detect.connection.looping.detection.threshold";
+ String PORT_CONNECTION_LOOPING_STOP_THRESHOLD =
"qpid.port.amqp.detect.connection.looping.stop.threshold";
Review comment:
As per comment above, the context variable name does not indicate that it is
only for TLS and "unwritten naming convention" is not followed...
What about changing the name to
`qpid.port.amqp.diagnosisOfSslEngineLoopingBreakThreshold` ?
Why two thresholds required?
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/transport/NonBlockingConnection.java
##########
@@ -691,4 +693,12 @@ public String getSelectedHost()
{
return _selectedHost;
}
+
+ public boolean detectConnectionLooping() { return
_port.getContextValue(Boolean.class, AmqpPort.PORT_DETECT_CONNECTION_LOOPING); }
+
+ public boolean isLooping() { return _loopingCounter.get() >
_port.getContextValue(Integer.class,
AmqpPort.PORT_CONNECTION_LOOPING_DETECTION_THRESHOLD); }
+
+ public boolean stopLooping() { return _loopingCounter.get() >=
_port.getContextValue(Integer.class,
AmqpPort.PORT_CONNECTION_LOOPING_STOP_THRESHOLD); }
Review comment:
It needs to be moved into `NonBlockingConnectionTLSDelegate` as it is
only used there.
A field can be added into `NonBlockingConnectionTLSDelegate` to hold a
value, as evaluation of context variable is expensive operation.
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/model/port/AmqpPort.java
##########
@@ -56,6 +56,10 @@
String PORT_AMQP_NUMBER_OF_SELECTORS =
"qpid.port.amqp.threadPool.numberOfSelectors";
String PORT_AMQP_ACCEPT_BACKLOG = "qpid.port.amqp.acceptBacklog";
+ String PORT_DETECT_CONNECTION_LOOPING =
"qpid.port.amqp.detect.connection.looping";
Review comment:
The intention of the JIRA is to detect unexpected loops in functionality
invoking SSLEngine wrap and possibly unwrap.
Thus, the context variable name need to convey that it is for TLS transport
only. Otherwise, the name can mislead that it applies to non-TLS transport.
Additionally, there is unwritten context variable naming convention. It
should be really added into documentation, as a lot of variables are named in
different ways.
Anyway the context variable convention is like below
qpid.<configured object category>.<configured object type>.<camelCasedName>
Thus, I would like to suggest the following name for the context variable
`qpid.port.amqp.enableDiagnosisOfSslEngineLooping`
IMHO, the above conveys that context variable applies only to SSL transport
and it is only for diagnostic purposes.
I am happy to change the name to any other name provided that the name
clearly indicate the purpose of context variable
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/transport/NonBlockingConnection.java
##########
@@ -75,6 +76,7 @@
private final AtomicLong _maxReadIdleMillis = new AtomicLong();
private final List<SchedulingDelayNotificationListener>
_schedulingDelayNotificationListeners = new CopyOnWriteArrayList<>();
private final AtomicBoolean _hasShutdown = new AtomicBoolean();
+ private final AtomicInteger _loopingCounter = new AtomicInteger(0);
Review comment:
The counter is only incremented in TLS delegate. Please move it into
`NonBlockingConnectionTLSDelegate`
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/transport/NonBlockingConnection.java
##########
@@ -691,4 +693,12 @@ public String getSelectedHost()
{
return _selectedHost;
}
+
+ public boolean detectConnectionLooping() { return
_port.getContextValue(Boolean.class, AmqpPort.PORT_DETECT_CONNECTION_LOOPING); }
Review comment:
why method is public?
IMHO, package protected visibility is better for the method.
As it is only used in `NonBlockingConnectionTLSDelegate`, it make sense to
move it there...
The evaluation of the context variables can be added into
`NonBlockingConnectionTLSDelegate` constructor and stored in field, as context
variable evaluation is quite expensive operation. It impacts the performance...
The name of method\variable can be changed to
`enableDiagnosisOfSslEngineLooping`...
##########
File path:
broker-core/src/main/java/org/apache/qpid/server/transport/NonBlockingConnection.java
##########
@@ -691,4 +693,12 @@ public String getSelectedHost()
{
return _selectedHost;
}
+
+ public boolean detectConnectionLooping() { return
_port.getContextValue(Boolean.class, AmqpPort.PORT_DETECT_CONNECTION_LOOPING); }
+
+ public boolean isLooping() { return _loopingCounter.get() >
_port.getContextValue(Integer.class,
AmqpPort.PORT_CONNECTION_LOOPING_DETECTION_THRESHOLD); }
Review comment:
It needs to be moved into `NonBlockingConnectionTLSDelegate` as it is
only used there.
A field can be added into `NonBlockingConnectionTLSDelegate` to hold a
value, as evaluation of context variable is expensive operation.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
