chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114371581
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManager.java:
##########
@@ -81,14 +93,16 @@ public PlcConnection getConnection(String url) throws
PlcConnectionException {
try {
return leaseFuture.get(this.maxWaitTime.toMillis(),
TimeUnit.MILLISECONDS);
} catch (ExecutionException | InterruptedException | TimeoutException
e) {
+ connectionContainer.close();
+ connectionContainers.remove(url);
throw new PlcConnectionException("Error acquiring lease for
connection", e);
}
}
- public PlcConnection getConnection(String url, PlcAuthentication
authentication) throws PlcConnectionException {
- throw new PlcConnectionException("the cached driver manager currently
doesn't support authentication");
Review Comment:
I intentionally left away the autherntication as in general we couldn't have
two connections to the same plc with different authentication ... if we do
this, we should probably also add the authentication information to the key of
the map.
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
##########
@@ -92,15 +102,139 @@ public PlcReadRequest.Builder readRequestBuilder() {
if(connection == null) {
throw new PlcRuntimeException("Error using leased connection after
returning it to the cache.");
}
- return connection.readRequestBuilder();
+ final PlcReadRequest.Builder innerBuilder =
connection.readRequestBuilder();
Review Comment:
Could you please explain what this is needed for?
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManager.java:
##########
@@ -58,21 +58,33 @@ public CachedPlcConnectionManager(PlcConnectionManager
connectionManager, Durati
this.connectionContainers = new HashMap<>();
}
+ @Override
public PlcConnection getConnection(String url) throws
PlcConnectionException {
+ return getConnection(url,null);
+ }
+
+ @Override
+ public PlcConnection getConnection(String url, PlcAuthentication
authentication) throws PlcConnectionException {
ConnectionContainer connectionContainer;
synchronized (connectionContainers) {
connectionContainer = connectionContainers.get(url);
- if (connectionContainers.get(url) == null) {
+ if (connectionContainer == null || connectionContainer.isClosed())
{
LOG.debug("Creating new connection");
// Establish the real connection to the plc
- PlcConnection connection =
connectionManager.getConnection(url);
-
- // Crate a connection container to manage handling this
connection
- connectionContainer = new ConnectionContainer(connection,
maxLeaseTime);
+ PlcConnection connection;
+ if(authentication!=null) {
+ connection =
connectionManager.getConnection(url,authentication);
+ } else{
+ connection = connectionManager.getConnection(url);
+ }
+ connectionContainer = new
ConnectionContainer(connection,maxLeaseTime);
connectionContainers.put(url, connectionContainer);
} else {
LOG.debug("Reusing exising connection");
+ if(connectionContainer.getRawConnection()!=null &&
!connectionContainer.getRawConnection().isConnected()){
Review Comment:
Instead of exposing the raw connection, I think it might be better to create
a new container and not expose the inner connection.
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
##########
@@ -66,7 +76,7 @@ public void connect() throws PlcConnectionException {
@Override
public boolean isConnected() {
if(connection == null) {
- throw new PlcRuntimeException("Error using leased connection after
returning it to the cache.");
+ return false;
Review Comment:
Not quite sure this is the best way to do this. Because for me there's a
difference, if we're not connected or if we already gave back the handle. The
way I implemented it gives the user the explicit feedback, that he's doing
something bad. If we return false, he might think he just needs to re-connect.
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/messages/DefaultPlcWriteRequest.java:
##########
@@ -194,6 +196,9 @@ public PlcWriteRequest build() {
PlcTag tag = tagValues.getLeft().get();
Object[] value = tagValues.getRight();
PlcValue plcValue = valueHandler.newPlcValue(tag, value);
+ if(tag.getPlcValueType()== PlcValueType.NULL){
Review Comment:
As far as I understood it PlcValueType.NULL is used in cases where a value
can be null (Like in pointers etc.) ... so it seems as only if you set the type
according to the plcValue, if this is NULL ... should not be confused with
"null".
--
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]
