You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2023/01/17 11:29:01 UTC
[GitHub] [plc4x] chrisdutz opened a new pull request, #747: New Implementation of the Connection-Cache
chrisdutz opened a new pull request, #747:
URL: https://github.com/apache/plc4x/pull/747
New-implementation of the Connection Cache and removal of both the old connection-cache as well as the connection-pool.
In addition some types in the API module were updated:
- The PlcDriverManager was renamed to DefaultPlcDriverManager
- A new Type PlcDriverManager
- A new Type PlcConnectionManager
- PlcDriverManager contains a getConnectionManager() method
- PlcDriverManager contains a static method getDefault() which returns an instance of the DefaultDriverManager
- DefaultPlcDriverManager implements both interfaces
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072121894
##########
plc4j/tools/scraper-ng/pom.xml:
##########
@@ -0,0 +1,110 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ https://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4j-tools</artifactId>
+ <version>0.11.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>plc4j-scraper-ng</artifactId>
+
+ <name>PLC4J: Tools: Scraper (NG)</name>
+ <description>Utility to efficiently collect a large number of items on multiple devices by different triggers.</description>
+
+ <dependencies>
+ <!--Jackson-->
Review Comment:
maybe remove those outcommented deps
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072121595
##########
plc4j/tools/scraper/pom.xml:
##########
@@ -100,12 +96,6 @@
<version>0.11.0-SNAPSHOT</version>
<scope>test</scope>
</dependency>
- <dependency>
- <groupId>org.apache.plc4x</groupId>
- <artifactId>plc4j-connection-cache</artifactId>
- <version>0.11.0-SNAPSHOT</version>
- <scope>compile</scope>
- </dependency>
</dependencies>
</project>
Review Comment:
```suggestion
</project>
```
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072181255
##########
plc4j/examples/hello-ads-telemetry/pom.xml:
##########
@@ -40,13 +43,31 @@
</dependency>
<dependency>
- <groupId>org.apache.commons</groupId>
- <artifactId>commons-pool2</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.apache.commons</groupId>
- <artifactId>commons-lang3</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>log4j-over-slf4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <configuration>
+ <usedDependencies combine.children="append">
+ <usedDependency>org.slf4j:log4j-over-slf4j</usedDependency>
+ </usedDependencies>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
</project>
Review Comment:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206
--
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
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072191526
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcDriverManager.java:
##########
@@ -16,17 +16,25 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.plc4x.java.utils.connectionpool2;
+package org.apache.plc4x.java.api;
-import org.apache.plc4x.java.api.PlcConnection;
+import org.apache.plc4x.java.DefaultPlcDriverManager;
import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
-/**
- * Simple Factory Method which creates a new PlcConnection
- */
-@FunctionalInterface
-public interface PlcConnectionFactory {
+import java.util.Set;
+
+public interface PlcDriverManager {
+
+ static PlcDriverManager getDefault() {
+ return new DefaultPlcDriverManager();
+ }
+
+ Set<String> listDrivers();
+
+ PlcDriver getDriver(String protocolCode) throws PlcConnectionException;
+
+ PlcDriver getDriverForUrl(String url) throws PlcConnectionException;
- PlcConnection create() throws PlcConnectionException;
Review Comment:
Let's agree on not calling them "customers" as they are missing one important aspect of a typical customer relation.
I would say that "customers" have the chance to come forward and announce this would break things for them. I would consider it important to have backward compatibility once we are at 1.0.0. But I wouldn't object your proposal. Feel free to add it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072116126
##########
plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/protocol/AdsProtocolLogic.java:
##########
@@ -136,11 +136,11 @@ public void onConnect(ConversationContext<AmsTCPPacket> context) {
// otherwise just mark the connection as completed instantly.
setupAmsRouteFuture.whenComplete((unused, throwable) -> {
if (!configuration.isLoadSymbolAndDataTypeTables()) {
- future.completeExceptionally(new PlcConnectionException(
- "Lazy loading is generally planned, but not implemented yet. " +
- "If you are in need for this feature, please reach out to the community."));
- }
- //if (configuration.isLoadSymbolAndDataTypeTables()) {
+ context.fireConnected();
+// future.completeExceptionally(new PlcConnectionException(
Review Comment:
is the outcommmented code here an accident?
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072117311
##########
plc4j/examples/hello-ads-telemetry/src/main/resources/logback.xml:
##########
@@ -27,7 +27,7 @@
</encoder>
</appender>
- <root level="error">
+ <root level="info">
Review Comment:
```suggestion
<root level="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
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072146589
##########
plc4j/examples/hello-ads-telemetry/pom.xml:
##########
@@ -40,13 +43,31 @@
</dependency>
<dependency>
- <groupId>org.apache.commons</groupId>
- <artifactId>commons-pool2</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.apache.commons</groupId>
- <artifactId>commons-lang3</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>log4j-over-slf4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <configuration>
+ <usedDependencies combine.children="append">
+ <usedDependency>org.slf4j:log4j-over-slf4j</usedDependency>
+ </usedDependencies>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
</project>
Review Comment:
Seriously? Not willing to do a discussion on the usage of new-lines.
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072113699
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcConnectionManager.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.api;
+
+import org.apache.plc4x.java.api.authentication.PlcAuthentication;
+import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
+
+public interface PlcConnectionManager {
+
+ /**
+ * Connects to a PLC using the given plc connection string.
+ *
+ * @param url plc connection string.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url) throws PlcConnectionException;
+
+ /**
+ * Connects to a PLC using the given plc connection string using given authentication credentials.
+ *
+ * @param url plc connection string.
+ * @param authentication authentication credentials.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url, PlcAuthentication authentication) throws PlcConnectionException;
Review Comment:
@chrisdutz we should some day think about using the option pattern from Golang here to (the ones with the varargs options). This way we could combine the getConnection into one method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072120717
##########
plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/protocol/AdsProtocolLogic.java:
##########
@@ -136,11 +136,11 @@ public void onConnect(ConversationContext<AmsTCPPacket> context) {
// otherwise just mark the connection as completed instantly.
setupAmsRouteFuture.whenComplete((unused, throwable) -> {
if (!configuration.isLoadSymbolAndDataTypeTables()) {
- future.completeExceptionally(new PlcConnectionException(
- "Lazy loading is generally planned, but not implemented yet. " +
- "If you are in need for this feature, please reach out to the community."));
- }
- //if (configuration.isLoadSymbolAndDataTypeTables()) {
+ context.fireConnected();
+// future.completeExceptionally(new PlcConnectionException(
Review Comment:
No ... it's a first step to make it possible to still connect to an ADS without loading the symbol table (if you just use raw 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
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072144969
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcDriverManager.java:
##########
@@ -16,17 +16,25 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.plc4x.java.utils.connectionpool2;
+package org.apache.plc4x.java.api;
-import org.apache.plc4x.java.api.PlcConnection;
+import org.apache.plc4x.java.DefaultPlcDriverManager;
import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
-/**
- * Simple Factory Method which creates a new PlcConnection
- */
-@FunctionalInterface
-public interface PlcConnectionFactory {
+import java.util.Set;
+
+public interface PlcDriverManager {
+
+ static PlcDriverManager getDefault() {
+ return new DefaultPlcDriverManager();
+ }
+
+ Set<String> listDrivers();
+
+ PlcDriver getDriver(String protocolCode) throws PlcConnectionException;
+
+ PlcDriver getDriverForUrl(String url) throws PlcConnectionException;
- PlcConnection create() throws PlcConnectionException;
Review Comment:
What for? I mean ... we're obviously not using it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072118836
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManager.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.utils.cache;
+
+import org.apache.plc4x.java.DefaultPlcDriverManager;
+import org.apache.plc4x.java.api.PlcConnection;
+import org.apache.plc4x.java.api.PlcConnectionManager;
+import org.apache.plc4x.java.api.authentication.PlcAuthentication;
+import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.time.Duration;
+import java.util.*;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public class CachedPlcConnectionManager implements PlcConnectionManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(CachedPlcConnectionManager.class);
+
+ private final PlcConnectionManager connectionManager;
+ private final Duration maxLeaseTime;
+ private final Duration maxWaitTime;
+
+ private final Map<String, ConnectionContainer> connectionContainers;
+
+ public static Builder getBuilder() {
+ return new Builder(new DefaultPlcDriverManager());
+ }
+
+ public static Builder getBuilder(PlcConnectionManager connectionManager) {
+ return new Builder(connectionManager);
+ }
+
+ public CachedPlcConnectionManager(PlcConnectionManager connectionManager, Duration maxLeaseTime, Duration maxWaitTime) {
+ this.connectionManager = connectionManager;
+ this.maxLeaseTime = maxLeaseTime;
+ this.maxWaitTime = maxWaitTime;
+ this.connectionContainers = new HashMap<>();
+ }
+
+ public PlcConnection getConnection(String url) throws PlcConnectionException {
+ ConnectionContainer connectionContainer;
+ synchronized (connectionContainers) {
Review Comment:
an alternative here might be the use of the ConcurrentHashMap
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072120527
##########
plc4j/tools/connection-cache/src/test/resources/logback-test.xml:
##########
@@ -27,7 +27,9 @@
</encoder>
</appender>
- <root level="error">
+ <logger name="org.apache.plc4x.java.utils.cache" level="debug"/>
+
+ <root level="info">
Review Comment:
```suggestion
<root level="error">
```
##########
plc4j/tools/connection-cache/src/test/resources/logback-test.xml:
##########
@@ -27,7 +27,9 @@
</encoder>
</appender>
- <root level="error">
+ <logger name="org.apache.plc4x.java.utils.cache" level="debug"/>
Review Comment:
```suggestion
<logger name="org.apache.plc4x.java.utils.cache" level="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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072185474
##########
plc4j/examples/hello-ads-telemetry/src/main/resources/logback.xml:
##########
@@ -27,7 +27,7 @@
</encoder>
</appender>
- <root level="error">
+ <root level="info">
Review Comment:
Couple of months back I turned them all up to error as we just exceeded the logoutput because of the size of our build. Those changes where all in the test-logback.xml. Just noticed that this is the main logback file so that should be fine so nvm
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072183895
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcDriverManager.java:
##########
@@ -16,17 +16,25 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.plc4x.java.utils.connectionpool2;
+package org.apache.plc4x.java.api;
-import org.apache.plc4x.java.api.PlcConnection;
+import org.apache.plc4x.java.DefaultPlcDriverManager;
import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
-/**
- * Simple Factory Method which creates a new PlcConnection
- */
-@FunctionalInterface
-public interface PlcConnectionFactory {
+import java.util.Set;
+
+public interface PlcDriverManager {
+
+ static PlcDriverManager getDefault() {
+ return new DefaultPlcDriverManager();
+ }
+
+ Set<String> listDrivers();
+
+ PlcDriver getDriver(String protocolCode) throws PlcConnectionException;
+
+ PlcDriver getDriverForUrl(String url) throws PlcConnectionException;
- PlcConnection create() throws PlcConnectionException;
Review Comment:
because it is part of the API package an might break exiting deployments from customers.
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072121286
##########
plc4j/tools/scraper-ng/pom.xml:
##########
@@ -0,0 +1,110 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ https://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ -->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+
+ <modelVersion>4.0.0</modelVersion>
+
+ <parent>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4j-tools</artifactId>
+ <version>0.11.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>plc4j-scraper-ng</artifactId>
+
+ <name>PLC4J: Tools: Scraper (NG)</name>
+ <description>Utility to efficiently collect a large number of items on multiple devices by different triggers.</description>
+
+ <dependencies>
+ <!--Jackson-->
+ <!--dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-annotations</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.dataformat</groupId>
+ <artifactId>jackson-dataformat-yaml</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.core</groupId>
+ <artifactId>jackson-databind</artifactId>
+ </dependency-->
+
+ <!-- Apache Commons -->
+ <!--dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-math3</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-pool2</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-collections4</artifactId>
+ </dependency-->
+
+ <!-- PLC4X dependencies -->
+ <!--dependency>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4j-api</artifactId>
+ <version>0.11.0-SNAPSHOT</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4j-connection-pool</artifactId>
+ <version>0.11.0-SNAPSHOT</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4j-connection-cache</artifactId>
+ <version>0.11.0-SNAPSHOT</version>
+ </dependency-->
+
+ <dependency>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4j-spi</artifactId>
+ <version>0.11.0-SNAPSHOT</version>
+ <scope>test</scope>
+ </dependency>
+
+ <!-- Test dependencies-->
+ <dependency>
+ <groupId>org.awaitility</groupId>
+ <artifactId>awaitility</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.plc4x</groupId>
+ <artifactId>plc4j-driver-mock</artifactId>
+ <version>0.11.0-SNAPSHOT</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+
+</project>
Review Comment:
```suggestion
</project>
```
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072120180
##########
plc4j/tools/connection-cache/src/test/java/org/apache/plc4x/java/utils/cache/CachedPlcConnectionManagerTest.java:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.utils.cache;
+
+import org.apache.plc4x.java.api.PlcConnection;
+import org.apache.plc4x.java.api.PlcConnectionManager;
+import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
+import org.apache.plc4x.java.api.exceptions.PlcRuntimeException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.time.Duration;
+
+public class CachedPlcConnectionManagerTest {
+
+ /**
+ * This is the simplest possible test. Here the ConnectionManager is used exactly once.
+ * So not really much of the caching we can test, but it tests if we're creating connections the right way.
+ *
+ * @throws PlcConnectionException something went wrong
+ */
+ @Test
+ public void testSingleConnectionRequestTest() throws PlcConnectionException {
+ PlcConnectionManager mockConnectionManager = Mockito.mock(PlcConnectionManager.class);
Review Comment:
There is also the Mockito Annotation which is pretty cool to use
--
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 Implementation of the Connection-Cache (plc4x)
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz merged PR #747:
URL: https://github.com/apache/plc4x/pull/747
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072115260
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcDriverManager.java:
##########
@@ -16,17 +16,25 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.plc4x.java.utils.connectionpool2;
+package org.apache.plc4x.java.api;
-import org.apache.plc4x.java.api.PlcConnection;
+import org.apache.plc4x.java.DefaultPlcDriverManager;
import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
-/**
- * Simple Factory Method which creates a new PlcConnection
- */
-@FunctionalInterface
-public interface PlcConnectionFactory {
+import java.util.Set;
+
+public interface PlcDriverManager {
+
+ static PlcDriverManager getDefault() {
+ return new DefaultPlcDriverManager();
+ }
+
+ Set<String> listDrivers();
+
+ PlcDriver getDriver(String protocolCode) throws PlcConnectionException;
+
+ PlcDriver getDriverForUrl(String url) throws PlcConnectionException;
- PlcConnection create() throws PlcConnectionException;
Review Comment:
I wonder if we should leave the create method as deprecated default implementation:
```
default PlcConnection create() {
return getConnectionManager().create();
}
```
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072112458
##########
plc4j/api/src/main/java/org/apache/plc4x/java/DefaultPlcDriverManager.java:
##########
@@ -32,19 +34,19 @@
import java.util.ServiceLoader;
import java.util.Set;
-public class PlcDriverManager {
+public class DefaultPlcDriverManager implements PlcDriverManager, PlcConnectionManager {
- private static final Logger LOGGER = LoggerFactory.getLogger(PlcDriverManager.class);
+ private static final Logger LOGGER = LoggerFactory.getLogger(org.apache.plc4x.java.DefaultPlcDriverManager.class);
Review Comment:
```suggestion
private static final Logger LOGGER = LoggerFactory.getLogger(DefaultPlcDriverManager.class);
```
--
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
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072116329
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcConnectionManager.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.api;
+
+import org.apache.plc4x.java.api.authentication.PlcAuthentication;
+import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
+
+public interface PlcConnectionManager {
+
+ /**
+ * Connects to a PLC using the given plc connection string.
+ *
+ * @param url plc connection string.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url) throws PlcConnectionException;
+
+ /**
+ * Connects to a PLC using the given plc connection string using given authentication credentials.
+ *
+ * @param url plc connection string.
+ * @param authentication authentication credentials.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url, PlcAuthentication authentication) throws PlcConnectionException;
Review Comment:
Well that is incompatible with the try-with-resources pattern ...
--
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
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072116724
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcConnectionManager.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.api;
+
+import org.apache.plc4x.java.api.authentication.PlcAuthentication;
+import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
+
+public interface PlcConnectionManager {
+
+ /**
+ * Connects to a PLC using the given plc connection string.
+ *
+ * @param url plc connection string.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url) throws PlcConnectionException;
+
+ /**
+ * Connects to a PLC using the given plc connection string using given authentication credentials.
+ *
+ * @param url plc connection string.
+ * @param authentication authentication credentials.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url, PlcAuthentication authentication) throws PlcConnectionException;
Review Comment:
At least I think so
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072116783
##########
plc4j/examples/hello-ads-telemetry/pom.xml:
##########
@@ -40,13 +43,31 @@
</dependency>
<dependency>
- <groupId>org.apache.commons</groupId>
- <artifactId>commons-pool2</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
- <groupId>org.apache.commons</groupId>
- <artifactId>commons-lang3</artifactId>
+ <groupId>org.slf4j</groupId>
+ <artifactId>log4j-over-slf4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
</dependency>
</dependencies>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <configuration>
+ <usedDependencies combine.children="append">
+ <usedDependency>org.slf4j:log4j-over-slf4j</usedDependency>
+ </usedDependencies>
+ </configuration>
+ </plugin>
+ </plugins>
+ </build>
+
</project>
Review Comment:
```suggestion
</project>
```
--
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
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072148900
##########
plc4j/examples/hello-ads-telemetry/src/main/resources/logback.xml:
##########
@@ -27,7 +27,7 @@
</encoder>
</appender>
- <root level="error">
+ <root level="info">
Review Comment:
I'm using the logging to output information on INFO level ... what good would the application be, if we set the log level to Error and just stripped all usable output? If yes, then you should have suggested a logger entry to explicitly set the log level of the application to info, but this change doesn't make any sense to me.
--
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
[GitHub] [plc4x] sruehl commented on a diff in pull request #747: New Implementation of the Connection-Cache
Posted by GitBox <gi...@apache.org>.
sruehl commented on code in PR #747:
URL: https://github.com/apache/plc4x/pull/747#discussion_r1072123552
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/PlcConnectionManager.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.api;
+
+import org.apache.plc4x.java.api.authentication.PlcAuthentication;
+import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
+
+public interface PlcConnectionManager {
+
+ /**
+ * Connects to a PLC using the given plc connection string.
+ *
+ * @param url plc connection string.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url) throws PlcConnectionException;
+
+ /**
+ * Connects to a PLC using the given plc connection string using given authentication credentials.
+ *
+ * @param url plc connection string.
+ * @param authentication authentication credentials.
+ * @return PlcConnection object.
+ * @throws PlcConnectionException an exception if the connection attempt failed.
+ */
+ PlcConnection getConnection(String url, PlcAuthentication authentication) throws PlcConnectionException;
Review Comment:
Don't think that should be a Problem but maybe I don't see it :D
--
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