You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by "spnettec (via GitHub)" <gi...@apache.org> on 2023/02/22 10:21:05 UTC
[PR] New various Changes for #1 (plc4x)
spnettec opened a new pull request, #818:
URL: https://github.com/apache/plc4x/pull/818
fix OutOfMemoryError
add Timeout-Handling
fix connection-cache
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by youlin he <he...@gmail.com>.
I want to say that a lot of code I don't think makes sense, but mine is too
many modified and hard to create a separate PR
spnettec (via GitHub) <gi...@apache.org> 于2023年2月23日周四 22:05写道：
>
> spnettec commented on code in PR #818:
> URL: https://github.com/apache/plc4x/pull/818#discussion_r1115744723
>
>
> ##########
>
> plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
> ##########
> @@ -51,13 +51,23 @@ public void run() {
>
> @Override
> public synchronized void close() {
> - // Make the connection unusable.
> - connection = null;
> -
> + if(connectionContainer == null) {
> + return;
> + }
>
> Review Comment:
> This is a bug. It should not throw an error. If you change usageTimer
> from local variable to private variable i can canel the Time. But... I
> still think the Timer is not necessary. So i don't do anyting. Just add
> this code.
>
>
>
> --
> 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: dev-unsubscribe@plc4x.apache.org
>
> For queries about this service, please contact Infrastructure at:
> users@infra.apache.org
>
>
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115459466
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
##########
@@ -51,13 +51,23 @@ public void run() {
@Override
public synchronized void close() {
- // Make the connection unusable.
- connection = null;
-
+ if(connectionContainer == null) {
+ return;
+ }
Review Comment:
Close twice. eg: user close this connection but usageTimer will close it again. It will cause nullpointexception
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115310805
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
##########
@@ -51,13 +51,23 @@ public void run() {
@Override
public synchronized void close() {
- // Make the connection unusable.
- connection = null;
-
+ if(connectionContainer == null) {
+ return;
+ }
Review Comment:
Why should the connectionContainer ever be null? The way I understand it, the only way this could happen, would be that a program got a lease ... executed a request, that had an issue and therefore it automatically closed the connection and destroy() called close from the inside and then the using application calls close regularly ... not too happy with this workflow in general ... we should really discuss things on the list.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115297104
##########
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 && value!=null) {
+ tag.setPlcValueType(plcValue.getPlcValueType());
+ }
Review Comment:
I still don't think we should have this in here ... if the tag says it's one type, but the PlcValue says it's something else, we shouldn't update this (I guess this is the reason you needed to add the "setPlcValue"). We should fire an exception, because in this case the user is doing something incorrectly.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115451257
##########
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 && value!=null) {
+ tag.setPlcValueType(plcValue.getPlcValueType());
+ }
Review Comment:
Please remove this from this PR for now as we have to deal with this "feature" separately.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115664628
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
##########
@@ -51,13 +51,23 @@ public void run() {
@Override
public synchronized void close() {
- // Make the connection unusable.
- connection = null;
-
+ if(connectionContainer == null) {
+ return;
+ }
Review Comment:
Yeah ... when returning a connection after a connection timed-out or got another error, it would have thrown an error from inside the returnConnection method of the Container.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115312253
##########
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();
+ return new PlcReadRequest.Builder(){
+
+ @Override
+ public PlcReadRequest build() {
+ final PlcReadRequest innerPlcReadRequest = innerBuilder.build();
+ return new PlcReadRequest(){
+
+ @Override
+ public CompletableFuture<? extends PlcReadResponse> execute() {
+ CompletableFuture<? extends PlcReadResponse> future = innerPlcReadRequest.execute();
+ final CompletableFuture<PlcReadResponse> responseFuture = new CompletableFuture<>();
+ future.handle((plcReadResponse, throwable) -> {
+ if (plcReadResponse != null) {
+ responseFuture.complete(plcReadResponse);
+ } else {
+ try {
+ destroy();
+ } catch (Exception e) {
+ }
+ responseFuture.completeExceptionally(throwable);
Review Comment:
But I guess ... otherwise there's no way for the user to actually invalidate a connection ... I guess we should probably have different sets of Exceptions that we return. Some that indicate the connection is no longer usable and some that say: you did something wrong, please do it right next time (Like invalid addresses)
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115725210
##########
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:
Just connect is not allowed. I think isConeccted is should reserve the functionality of the original API
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115305690
##########
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();
+ return new PlcReadRequest.Builder(){
+
+ @Override
+ public PlcReadRequest build() {
+ final PlcReadRequest innerPlcReadRequest = innerBuilder.build();
+ return new PlcReadRequest(){
+
+ @Override
+ public CompletableFuture<? extends PlcReadResponse> execute() {
+ CompletableFuture<? extends PlcReadResponse> future = innerPlcReadRequest.execute();
+ final CompletableFuture<PlcReadResponse> responseFuture = new CompletableFuture<>();
+ future.handle((plcReadResponse, throwable) -> {
+ if (plcReadResponse != null) {
Review Comment:
Could we change this to "throwable == null"? ... it generally means the same, but it more sets the focus on "nothing went wrong".
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115500266
##########
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:
We use PLC4x to collect data for long term work. If the connection is closed abnormally, I don’t want to always use the try catch method. If we use this to detect that the connection is closed, we only need to borrow it again.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "hongjinlin (via GitHub)" <gi...@apache.org>.
hongjinlin commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115166844
##########
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:
> I have already explained that previous PR are the basis for subsequent PRs. Next I want to submit a PR for PL4x
First of all, thank you for your PR. Secondly, I think it would be beneficial if you could discuss your entire design approach with everyone, rather than just submitting a basic PR and following up with subsequent PRs.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115453746
##########
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();
+ return new PlcReadRequest.Builder(){
+
+ @Override
+ public PlcReadRequest build() {
+ final PlcReadRequest innerPlcReadRequest = innerBuilder.build();
+ return new PlcReadRequest(){
+
+ @Override
+ public CompletableFuture<? extends PlcReadResponse> execute() {
+ CompletableFuture<? extends PlcReadResponse> future = innerPlcReadRequest.execute();
+ final CompletableFuture<PlcReadResponse> responseFuture = new CompletableFuture<>();
+ future.handle((plcReadResponse, throwable) -> {
+ if (plcReadResponse != null) {
+ responseFuture.complete(plcReadResponse);
+ } else {
+ try {
+ destroy();
+ } catch (Exception e) {
+ }
+ responseFuture.completeExceptionally(throwable);
Review Comment:
Invalid tag is not throw exception. It is normal return result. This is unexcepted exception not caught by protocol. So this error is very serious network error or protocol data error
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115431392
##########
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);
Review Comment:
I never use authentication. Just follow the api. I can remove this change
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115313507
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/model/PlcTag.java:
##########
@@ -63,6 +63,10 @@ default PlcValueType getPlcValueType() {
return PlcValueType.NULL;
}
+ @JsonIgnore
+ default void setPlcValueType(PlcValueType plcValueType){
+ }
Review Comment:
This seems to be related to the change in DefaultPlcWrite request, were the plc value type can be updated ... as I don't really like that change, I think we should also remove this change ... as both actually don't have anything to do with what we would like to address with this PR
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115439715
##########
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();
+ return new PlcReadRequest.Builder(){
+
+ @Override
+ public PlcReadRequest build() {
+ final PlcReadRequest innerPlcReadRequest = innerBuilder.build();
+ return new PlcReadRequest(){
+
+ @Override
+ public CompletableFuture<? extends PlcReadResponse> execute() {
+ CompletableFuture<? extends PlcReadResponse> future = innerPlcReadRequest.execute();
+ final CompletableFuture<PlcReadResponse> responseFuture = new CompletableFuture<>();
+ future.handle((plcReadResponse, throwable) -> {
+ if (plcReadResponse != null) {
Review Comment:
Right. Thank you
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115434878
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java:
##########
@@ -54,8 +62,19 @@ public synchronized Future<PlcConnection> lease() {
}
return connectionFuture;
}
-
+ public synchronized void close(){
+ CompletableFuture<PlcConnection> leaseFuture;
+ while((leaseFuture = queue.poll())!=null){
+ leaseFuture.complete(null);
Review Comment:
I agree that this should throw exception
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115427998
##########
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 && value!=null) {
+ tag.setPlcValueType(plcValue.getPlcValueType());
+ }
Review Comment:
It can remove tag formate that depends on other driver's label formats.
I can remove this change. Next PR. I want to fix plc4x driver and server
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115468272
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/model/PlcTag.java:
##########
@@ -63,6 +63,10 @@ default PlcValueType getPlcValueType() {
return PlcValueType.NULL;
}
+ @JsonIgnore
+ default void setPlcValueType(PlcValueType plcValueType){
+ }
Review Comment:
Ok. I will rollback
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115456529
##########
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();
+ return new PlcReadRequest.Builder(){
+
+ @Override
+ public PlcReadRequest build() {
+ final PlcReadRequest innerPlcReadRequest = innerBuilder.build();
+ return new PlcReadRequest(){
+
+ @Override
+ public CompletableFuture<? extends PlcReadResponse> execute() {
+ CompletableFuture<? extends PlcReadResponse> future = innerPlcReadRequest.execute();
+ final CompletableFuture<PlcReadResponse> responseFuture = new CompletableFuture<>();
+ future.handle((plcReadResponse, throwable) -> {
+ if (plcReadResponse != null) {
+ responseFuture.complete(plcReadResponse);
+ } else {
+ try {
+ destroy();
+ } catch (Exception e) {
+ }
+ responseFuture.completeExceptionally(throwable);
Review Comment:
Ok ... I think you are right with this. So please ignore my comment.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114532155
##########
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:
I have already explained that previous PR are the basis for subsequent PRs. Next I want to submit a PR for PL4x
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114527206
##########
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:
By the way. The value can be set null not PlcValueType be set NULL. PLC4x driver write request must give explicit data type. This limits all the driver write tag format for the PLC4x
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115291492
##########
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:
Well I'd rather simply do it right, right away ... shouldn't be too much additional work ... sort of like a "url + authentication.toString()"
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115656432
##########
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:
I would say that's a usecase particular to how you use PLC4X, however out API shouldn't be optimized towards one particular usecase. I think try-catch-blocks don't consume any noticable amount of resources. So I would like to ask to to stick to using try-catch blocks and us keeping the API coherent.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115292722
##########
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:
I still don't quite understand the problem with this ... if you returned the connection to the pool, you should not do anything with it ... that's sort of the contract on using the pool. If there's some logic calling "isConnected()" after it has been returned, that's just a flaw in the program using it. I still think we should continue to throw an exception 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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115422128
##########
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 && value!=null) {
+ tag.setPlcValueType(plcValue.getPlcValueType());
+ }
Review Comment:
I want to change plc4x tagAddress can praser other driver. eg: %DB2:94:STRING(10), %DB2:78:LREAL[2]
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114492739
##########
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:
Sometimes the row connection will die but we can't detected.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #818:
URL: https://github.com/apache/plc4x/pull/818#issuecomment-1439797734
YESS! ... that's much more something we can discuss ... I'll do that as soon as I can (Currently not allowed to do Apache code-work while on the clock :-( ) ... but just wanted to say that it's greatly appreciated :-)
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114444171
##########
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:
Also admittedly ... I'm not 100% sure if all drivers safely allow reconnecting if they were somehow disconnected.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114494692
##########
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:
Just leave it for the future to authorize
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114516266
##########
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:
By the way. I'm working on eclipse kura. It require work uninterrupted. We must have explicit reconect signal to reconnect not a exception.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114486107
##########
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:
This is ready for pl4x driver
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115308098
##########
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();
+ return new PlcReadRequest.Builder(){
+
+ @Override
+ public PlcReadRequest build() {
+ final PlcReadRequest innerPlcReadRequest = innerBuilder.build();
+ return new PlcReadRequest(){
+
+ @Override
+ public CompletableFuture<? extends PlcReadResponse> execute() {
+ CompletableFuture<? extends PlcReadResponse> future = innerPlcReadRequest.execute();
+ final CompletableFuture<PlcReadResponse> responseFuture = new CompletableFuture<>();
+ future.handle((plcReadResponse, throwable) -> {
+ if (plcReadResponse != null) {
+ responseFuture.complete(plcReadResponse);
+ } else {
+ try {
+ destroy();
+ } catch (Exception e) {
+ }
+ responseFuture.completeExceptionally(throwable);
Review Comment:
So do I understand this block correctly? If anything goes wrong with the request, it terminates the connection, right? Should we really be doing this? I mean ... it's easy to ask for an invalid tag ... that doesn't really make it necessary to reconnect? For example with my KNX gateway it only allows 3 connections in paralell and within a window of 2-3 minutes (when it closes inactive connections) ... not 100% sure we should be doing this.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115294277
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/model/PlcTag.java:
##########
@@ -63,6 +63,10 @@ default PlcValueType getPlcValueType() {
return PlcValueType.NULL;
}
+ @JsonIgnore
+ default void setPlcValueType(PlcValueType plcValueType){
+ }
Review Comment:
Hmm ... this adds a default implementation, which doesn't really do anything. Also do we usually implement all of the types in the API module, to create immutable types. Is there any reason for this addition? I think our tags should generally stay immutable types.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114488571
##########
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:
This is for handling request failure and close the raw connection
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114490721
##########
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:
If we lose the connection we can handle reconnection
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on PR #818:
URL: https://github.com/apache/plc4x/pull/818#issuecomment-1440387183
The raw conection is netty conection and if disconnected it always can
reconnect
Christofer Dutz ***@***.***> 于2023年2月22日周三 22:57写道：
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In
> plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManager.java
> <https://github.com/apache/plc4x/pull/818#discussion_r1114444171>:
>
> > connectionContainers.put(url, connectionContainer);
> } else {
> LOG.debug("Reusing exising connection");
> + if(connectionContainer.getRawConnection()!=null && !connectionContainer.getRawConnection().isConnected()){
>
> Also admittedly ... I'm not 100% sure if all drivers safely allow
> reconnecting if they were somehow disconnected.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/plc4x/pull/818#discussion_r1114444171>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AANP4ZB7FQLGUAQMR672INDWYYSOPANCNFSM6AAAAAAVEEMXEI>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "hongjinlin (via GitHub)" <gi...@apache.org>.
hongjinlin commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115163118
##########
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:
>
>
I have encountered a scenario where the PLC connection appeared to be disconnected, but in reality it was still connected due to certain reasons. However, we kept trying to reconnect and as a result, the TCP estab value increased to tens of thousands. This caused the PLC to be disconnected and also resulted in server network issues. Therefore, I'm looking to see if there is a better solution available.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115452529
##########
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);
Review Comment:
Well then we should leave it as it was ... either we support it and then we should really support it, or we don't and we throw an exception (as it was before)
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115455299
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/model/PlcTag.java:
##########
@@ -63,6 +63,10 @@ default PlcValueType getPlcValueType() {
return PlcValueType.NULL;
}
+ @JsonIgnore
+ default void setPlcValueType(PlcValueType plcValueType){
+ }
Review Comment:
Yes please ... as I mentioned, we should try to keep one PR separated from the other ... we're already mixing up the Cache and the Timeouts (which I think is ok as I think both are not controversial)
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115445285
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/model/PlcTag.java:
##########
@@ -63,6 +63,10 @@ default PlcValueType getPlcValueType() {
return PlcValueType.NULL;
}
+ @JsonIgnore
+ default void setPlcValueType(PlcValueType plcValueType){
+ }
Review Comment:
This is ready for PLC4X Driver. Maybe i should change these in next PR
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115730021
##########
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:
I'm wondering why you want to disable this API functionality and replace it with an error
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115294277
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/model/PlcTag.java:
##########
@@ -63,6 +63,10 @@ default PlcValueType getPlcValueType() {
return PlcValueType.NULL;
}
+ @JsonIgnore
+ default void setPlcValueType(PlcValueType plcValueType){
+ }
Review Comment:
Hmm ... this adds a default implementation, which doesn't really do anything. Also do we usually implement all of the types in the API module, to create immutable types. Is there any reason for this addition?
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115300155
##########
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);
Review Comment:
As this now includes the authentication, we should probably have a more generic "getContainerId" method, that takes a url and authentication and if authentication is not null, then it sort of does a "url + authentication.toString()" to allow multiple connections with different users (I could imagine this being a topic in OPC-UA)
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115301187
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java:
##########
@@ -28,12 +28,20 @@
import java.util.concurrent.Future;
class ConnectionContainer {
- private final PlcConnection connection;
+ private PlcConnection connection;
+ private boolean closed = false;
private final Duration maxLeaseTime;
private final Queue<CompletableFuture<PlcConnection>> queue;
private LeasedPlcConnection leasedConnection;
+ public boolean isClosed() {
+ return closed;
+ }
+ public PlcConnection getRawConnection() {
Review Comment:
In contrast to your comment, this does actually return the Plc4x connection and not the Netty connection ... we shouldn't expose this, as this allows "stealing" the connection.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115303543
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/ConnectionContainer.java:
##########
@@ -54,8 +62,19 @@ public synchronized Future<PlcConnection> lease() {
}
return connectionFuture;
}
-
+ public synchronized void close(){
+ CompletableFuture<PlcConnection> leaseFuture;
+ while((leaseFuture = queue.poll())!=null){
+ leaseFuture.complete(null);
Review Comment:
Wouldn't it make more sense to complete exceptionally instead of returning null? We are currently expecting the result of a getConnection() to be a usable connection or an exception ... returning "null" sort of doesn't match that contract. I would change it to "completeExceptionally"
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1115744723
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
##########
@@ -51,13 +51,23 @@ public void run() {
@Override
public synchronized void close() {
- // Make the connection unusable.
- connection = null;
-
+ if(connectionContainer == null) {
+ return;
+ }
Review Comment:
This is a bug. It should not throw an error. If you change usageTimer from local variable to private variable i can canel the Time. But... I still think the Timer is not necessary. So i don't do anyting. Just add this code.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114516266
##########
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:
By the way. I'm working on eclipse kura. It required working uninterrupted. We must have explicit reconect signal to reconnect not a exception.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114516266
##########
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:
By the way. I'm working on eclipse kura. It required working uninterrupted. We must explicit reconect signal to reconnect not a exception.
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec commented on code in PR #818:
URL: https://github.com/apache/plc4x/pull/818#discussion_r1114529267
##########
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:
You can checkout my repo and take look PL4x driver and PL4x server
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] New various Changes for #1 (plc4x)
Posted by "spnettec (via GitHub)" <gi...@apache.org>.
spnettec closed pull request #818: New various Changes for #1
URL: https://github.com/apache/plc4x/pull/818
--
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: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org