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/19 14:49:50 UTC
[GitHub] [plc4x] QuanticPony opened a new pull request, #755: Feature/nifi integration address text
QuanticPony opened a new pull request, #755:
URL: https://github.com/apache/plc4x/pull/755
Proposal to add a property in NiFi-integration processors to allow users to choose address definition strategy and add support for Expression Language in addresses definition.
## Issues related:
* #648: As stated, adding a lot of addresses as dynamic properties can be tedious.
* #593 (Jira [PLC4X-196](https://issues.apache.org/jira/browse/PLC4X-196)): Expression Language can be supported easily on addresses definition.
## Actual behavior:
Addresses are specified one by one as dynamic properties in the processor and stored in a map when `onScheduled` method is triggered.
Expression Language is not supported.
## Proposed behavior:
Let user decide between 2 strategies to obtain addresses in the processor:
* Properties as addresses (as actual behavior but with Expression Language): 1 address per dynamic property.
* Property "Address Text": a property where all addresses can be specified as field-value pairs in a JSON.
An address map created every time when `onTrigger` method is called.
--
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] Feature/nifi integration address text (plc4x)
Posted by "ottobackwards (via GitHub)" <gi...@apache.org>.
ottobackwards commented on code in PR #755:
URL: https://github.com/apache/plc4x/pull/755#discussion_r1088235056
##########
plc4j/integrations/apache-nifi/nifi-plc4x-processors/src/main/java/org/apache/plc4x/nifi/record/Plc4xReadResponseRecordSet.java:
##########
@@ -58,9 +58,10 @@ public Plc4xReadResponseRecordSet(final PlcReadResponse readResponse) throws IOE
rsColumnNames = responseDataStructure.keySet();
if (recordSchema == null) {
Review Comment:
Could we not cache here as well?
##########
plc4j/integrations/apache-nifi/nifi-plc4x-processors/src/main/java/org/apache/plc4x/nifi/record/Plc4xReadResponseRecordSet.java:
##########
@@ -47,9 +47,9 @@ public class Plc4xReadResponseRecordSet implements RecordSet, Closeable {
Review Comment:
Are these TODO's still valid?
##########
plc4j/integrations/apache-nifi/nifi-plc4x-processors/src/main/java/org/apache/plc4x/nifi/record/SchemaCache.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.nifi.record;
+
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReferenceArray;
+
+import org.apache.nifi.serialization.record.RecordSchema;
+import org.apache.plc4x.java.api.model.PlcTag;
+
+public class SchemaCache {
+ private ConcurrentMap<String, SchemaContainer> schemaMap = new ConcurrentHashMap<>();
+ private AtomicReferenceArray<String> schemaAppendOrder = new AtomicReferenceArray<>(0);
+ private final AtomicInteger lastSchemaPosition = new AtomicInteger(0);
+ private final AtomicInteger cacheSize = new AtomicInteger(0);
+
+ public SchemaCache(int cacheSize) {
+ this.cacheSize.set(cacheSize);
+ }
+
+ public void setCacheSize(int cacheSize) {
+ this.cacheSize.set(cacheSize);
+ schemaAppendOrder = new AtomicReferenceArray<>(cacheSize);
+ schemaMap = new ConcurrentHashMap<>();
+ }
+
Review Comment:
We need tests for this class to ensure it works properly.
Some JavaDoc would be nice here too.
Also it would be nice if there were some logging here or elsewhere of cache misses etc, in DEBUG mode/level
--
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] Feature/nifi integration address text (plc4x)
Posted by "QuanticPony (via GitHub)" <gi...@apache.org>.
QuanticPony commented on PR #755:
URL: https://github.com/apache/plc4x/pull/755#issuecomment-1424138394
Must be a left over from testing. There is not any reason, sorry for the inconvenience
--
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] Feature/nifi integration address text (plc4x)
Posted by "hutcheb (via GitHub)" <gi...@apache.org>.
hutcheb commented on PR #755:
URL: https://github.com/apache/plc4x/pull/755#issuecomment-1424126398
Just going through and removing jackson being pinned at version 2.14.1, this will result in it being bumped to 2.14.2.
This was causing an issue with the build. Let me know if there was a specific reason this was being held back.
--
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] ottobackwards commented on pull request #755: Feature/nifi integration address text
Posted by GitBox <gi...@apache.org>.
ottobackwards commented on PR #755:
URL: https://github.com/apache/plc4x/pull/755#issuecomment-1398554496
Thanks for the contribution!
The issues with expression language for addresses and having multiple addresses in that property were at the time:
- Putting something complicated to write in a text field is poor for the user
- The original didn't use json, but used special character substitution changing the native addresses which was a no-go
- The addresses / returns need to have their PLC types resolved, and if the address could change _every_ call ( which you have to assume if you use expression language ) then it would be good to have some kind of caching so that it wasn't so costly every time, and the original PR did not provide that
*This PR to land would have to address the caching issue*. Having json addresses the "don't invent a custom addressing delimiter" issue. We would just have to live with how poor writing complicated json in that UI would be ( or doing it external and cut and pasting it in)
--
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] Feature/nifi integration address text (plc4x)
Posted by "ottobackwards (via GitHub)" <gi...@apache.org>.
ottobackwards commented on code in PR #755:
URL: https://github.com/apache/plc4x/pull/755#discussion_r1088281508
##########
plc4j/integrations/apache-nifi/nifi-plc4x-processors/src/main/java/org/apache/plc4x/nifi/record/Plc4xReadResponseRecordSet.java:
##########
@@ -47,9 +47,9 @@ public class Plc4xReadResponseRecordSet implements RecordSet, Closeable {
Review Comment:
Can you remove them please?
--
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] Feature/nifi integration address text (plc4x)
Posted by "QuanticPony (via GitHub)" <gi...@apache.org>.
QuanticPony commented on PR #755:
URL: https://github.com/apache/plc4x/pull/755#issuecomment-1405115814
@ottobackwards regarding the cache:
I have added a cache to store addresses-to-PlcTags and addresses-to-RecordSchema mappings.
I have done some tests and seems to speed up ut to 10x the processors.
At the moment each processor has its cache and a new property: *Schema Cache Size* with default value 1.
Waiting for feedback
--
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] Feature/nifi integration address text (plc4x)
Posted by "QuanticPony (via GitHub)" <gi...@apache.org>.
QuanticPony commented on code in PR #755:
URL: https://github.com/apache/plc4x/pull/755#discussion_r1088244336
##########
plc4j/integrations/apache-nifi/nifi-plc4x-processors/src/main/java/org/apache/plc4x/nifi/record/Plc4xReadResponseRecordSet.java:
##########
@@ -47,9 +47,9 @@ public class Plc4xReadResponseRecordSet implements RecordSet, Closeable {
Review Comment:
With the cache they would not be
--
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] Feature/nifi integration address text (plc4x)
Posted by "ottobackwards (via GitHub)" <gi...@apache.org>.
ottobackwards merged PR #755:
URL: https://github.com/apache/plc4x/pull/755
--
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] Feature/nifi integration address text (plc4x)
Posted by "QuanticPony (via GitHub)" <gi...@apache.org>.
QuanticPony commented on code in PR #755:
URL: https://github.com/apache/plc4x/pull/755#discussion_r1088246035
##########
plc4j/integrations/apache-nifi/nifi-plc4x-processors/src/main/java/org/apache/plc4x/nifi/record/Plc4xReadResponseRecordSet.java:
##########
@@ -58,9 +58,10 @@ public Plc4xReadResponseRecordSet(final PlcReadResponse readResponse) throws IOE
rsColumnNames = responseDataStructure.keySet();
if (recordSchema == null) {
Review Comment:
This part is already cached on the Plc4xSourceRecordProcessor L187 (https://github.com/zylklab/plc4x/blob/4871a0ca4fd3befc773fd624a33ef31372647751/plc4j/integrations/apache-nifi/nifi-plc4x-processors/src/main/java/org/apache/plc4x/nifi/Plc4xSourceRecordProcessor.java#L187)
--
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] Feature/nifi integration address text (plc4x)
Posted by "QuanticPony (via GitHub)" <gi...@apache.org>.
QuanticPony commented on PR #755:
URL: https://github.com/apache/plc4x/pull/755#issuecomment-1412196424
Thank you for your feedback @ottobackwards!
I have made commits addressing all suggested changes
--
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] Feature/nifi integration address text (plc4x)
Posted by "ottobackwards (via GitHub)" <gi...@apache.org>.
ottobackwards commented on PR #755:
URL: https://github.com/apache/plc4x/pull/755#issuecomment-1416773300
This looks good. If you are ready, I will let you move it from Draft to ready for review, then I'll finish up
--
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