Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
exceptionfactory closed pull request #7940: NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors URL: https://github.com/apache/nifi/pull/7940 -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
joewitt commented on PR #7940: URL: https://github.com/apache/nifi/pull/7940#issuecomment-2094285531 @ChrisSamo632 Thanks for rebasing and also leveraging migrate properties! -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
ChrisSamo632 commented on PR #7940: URL: https://github.com/apache/nifi/pull/7940#issuecomment-2094061873 Rebased against latest `main` to refactor changed into new `nifi-extension-bundles` module -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
exceptionfactory commented on code in PR #7940:
URL: https://github.com/apache/nifi/pull/7940#discussion_r1540029633
##
nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java:
##
@@ -229,33 +226,32 @@ public void run(final int iterations, final boolean
stopOnFinish, final boolean
executorService.awaitTermination(runWait,
TimeUnit.MILLISECONDS);
} catch (final InterruptedException e1) {
}
+}
-int finishedCount = 0;
-boolean unscheduledRun = false;
-for (final Future future : futures) {
-try {
-final Throwable thrown = future.get(); // wait for the
result
-if (thrown != null) {
-throw new AssertionError(thrown);
-}
+int finishedCount = 0;
+boolean unscheduledRun = false;
+for (final Future future : futures) {
+try {
+final Throwable thrown = future.get(); // wait for the result
+if (thrown != null) {
+throw new AssertionError(thrown);
+}
-if (++finishedCount == 1 && stopOnFinish) {
-unscheduledRun = true;
-unSchedule();
-}
-} catch (final Exception e) {
+if (++finishedCount == 1 && stopOnFinish) {
+unscheduledRun = true;
+unSchedule();
}
+} catch (final Exception e) {
+e.printStackTrace();
Review Comment:
Thanks, I think it is best to avoid the `printStackTrace()` for now.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
ChrisSamo632 commented on code in PR #7940:
URL: https://github.com/apache/nifi/pull/7940#discussion_r1539989744
##
nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java:
##
@@ -229,33 +226,32 @@ public void run(final int iterations, final boolean
stopOnFinish, final boolean
executorService.awaitTermination(runWait,
TimeUnit.MILLISECONDS);
} catch (final InterruptedException e1) {
}
+}
-int finishedCount = 0;
-boolean unscheduledRun = false;
-for (final Future future : futures) {
-try {
-final Throwable thrown = future.get(); // wait for the
result
-if (thrown != null) {
-throw new AssertionError(thrown);
-}
+int finishedCount = 0;
+boolean unscheduledRun = false;
+for (final Future future : futures) {
+try {
+final Throwable thrown = future.get(); // wait for the result
+if (thrown != null) {
+throw new AssertionError(thrown);
+}
-if (++finishedCount == 1 && stopOnFinish) {
-unscheduledRun = true;
-unSchedule();
-}
-} catch (final Exception e) {
+if (++finishedCount == 1 && stopOnFinish) {
+unscheduledRun = true;
+unSchedule();
}
+} catch (final Exception e) {
+e.printStackTrace();
Review Comment:
The `Exception` capture is nothing new, just previously it was being
silently ignored, so I thought I'd at least indicate that the `catch` had been
triggered, without potentially causing problems for lots of tests across the
existing estate
Happy to revert this and silently swallow the `InterruptedException` &
`ExecutionException`s, although I'll change it from catching all `Exception`s
at the least
This approach matches other similar handling elsewhere within the
`TestRunner`
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
exceptionfactory commented on code in PR #7940:
URL: https://github.com/apache/nifi/pull/7940#discussion_r1538546543
##
nifi-mock/src/main/java/org/apache/nifi/util/RelationshipMigrationResult.java:
##
@@ -0,0 +1,34 @@
+/*
+ * 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
+ *
+ * http://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.nifi.util;
+
+import java.util.Map;
+import java.util.Set;
+
+public interface RelationshipMigrationResult {
+
+/**
+ * @return a mapping of previous relationship names to the new names of
those relationships
+ */
+Map> getRelationshipsSplit();
+
+/**
+ * @return a mapping of previous relationship names to the new names of
those relationships
+ */
+Map getRelationshipsRenamed();
Review Comment:
Aligning with the other method, perhaps `getRenamedRelationships()`?
##
nifi-mock/src/main/java/org/apache/nifi/util/RelationshipMigrationResult.java:
##
@@ -0,0 +1,34 @@
+/*
+ * 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
+ *
+ * http://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.nifi.util;
+
+import java.util.Map;
+import java.util.Set;
+
+public interface RelationshipMigrationResult {
+
+/**
+ * @return a mapping of previous relationship names to the new names of
those relationships
+ */
+Map> getRelationshipsSplit();
Review Comment:
The `Split` naming is not quite clear. Perhaps `getPreviousRelationships()`?
##
nifi-mock/src/main/java/org/apache/nifi/util/StandardProcessorTestRunner.java:
##
@@ -229,33 +226,32 @@ public void run(final int iterations, final boolean
stopOnFinish, final boolean
executorService.awaitTermination(runWait,
TimeUnit.MILLISECONDS);
} catch (final InterruptedException e1) {
}
+}
-int finishedCount = 0;
-boolean unscheduledRun = false;
-for (final Future future : futures) {
-try {
-final Throwable thrown = future.get(); // wait for the
result
-if (thrown != null) {
-throw new AssertionError(thrown);
-}
+int finishedCount = 0;
+boolean unscheduledRun = false;
+for (final Future future : futures) {
+try {
+final Throwable thrown = future.get(); // wait for the result
+if (thrown != null) {
+throw new AssertionError(thrown);
+}
-if (++finishedCount == 1 && stopOnFinish) {
-unscheduledRun = true;
-unSchedule();
-}
-} catch (final Exception e) {
+if (++finishedCount == 1 && stopOnFinish) {
+unscheduledRun = true;
+unSchedule();
}
+} catch (final Exception e) {
+e.printStackTrace();
Review Comment:
This use of `printStackTrace()` seems problematic, better throw a more
particular type of 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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
ChrisSamo632 commented on PR #7940: URL: https://github.com/apache/nifi/pull/7940#issuecomment-1949279317 I've rebased from latest `main` to address merge conflicts. Let me know your thoughts on previous comments @exceptionfactory. -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
ChrisSamo632 commented on PR #7940: URL: https://github.com/apache/nifi/pull/7940#issuecomment-1897996225 RE the migration testing, I discussed this with @markap14 at the time. I find some issues with the property mock processing, so fixed that (I'll have to reverse engineer what the problem was now, if more details are needed). Adding the relationship mocking is something that could be separated out, although at the time it helped me confirm the updates I wanted to make and I thought it might be helpful for others in the future. It does, of course, increase this PR a bit though. Happy to create a separate jira ticket and either link it here (to reflect the additional change), or try to separate out the changes if people think it's necessary -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
ChrisSamo632 commented on code in PR #7940:
URL: https://github.com/apache/nifi/pull/7940#discussion_r1457069952
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractPutElasticsearch.java:
##
@@ -55,10 +55,41 @@
import java.util.stream.Collectors;
public abstract class AbstractPutElasticsearch extends AbstractProcessor
implements ElasticsearchRestProcessor {
+static final Relationship REL_ORIGINAL = new Relationship.Builder()
+.name("original")
+.description("All flowfiles that are sent to Elasticsearch without
request failures go to this relationship.")
+.build();
+
+static final Relationship REL_SUCCESSFUL = new Relationship.Builder()
+.name("successful")
Review Comment:
That, and I think renaming the relationship might impact other ES processors
(I'll have to double check)
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
ChrisSamo632 commented on code in PR #7940:
URL: https://github.com/apache/nifi/pull/7940#discussion_r1457066410
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractPutElasticsearch.java:
##
@@ -55,10 +55,41 @@
import java.util.stream.Collectors;
public abstract class AbstractPutElasticsearch extends AbstractProcessor
implements ElasticsearchRestProcessor {
+static final Relationship REL_ORIGINAL = new Relationship.Builder()
+.name("original")
+.description("All flowfiles that are sent to Elasticsearch without
request failures go to this relationship.")
+.build();
+
+static final Relationship REL_SUCCESSFUL = new Relationship.Builder()
+.name("successful")
Review Comment:
My thinking (from memory) is that `success` in nifi is usually the output of
the incoming FlowFile once it has passed through the processor without error
Here, the semantics are different in that this relationship will contain the
(potentially reformatted) record(s)/json that has been processed within
elasticsearch without error. That is, sent to the ES _bulk endpoint and ES has
reported the operation as successful without error.
ES docs that fail on the ES side are sent to the errors output in nifi (so
the original incoming FlowFile may end up being split between two outputs, even
though the "send" to ES was a success).
So separating the two seemed reasonable. That said, I'm not against using
success for consistency in banking, provided the difference in semantics won't
lead to greater confusion.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
exceptionfactory commented on code in PR #7940:
URL: https://github.com/apache/nifi/pull/7940#discussion_r1456861650
##
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-restapi-processors/src/main/java/org/apache/nifi/processors/elasticsearch/AbstractPutElasticsearch.java:
##
@@ -55,10 +55,41 @@
import java.util.stream.Collectors;
public abstract class AbstractPutElasticsearch extends AbstractProcessor
implements ElasticsearchRestProcessor {
+static final Relationship REL_ORIGINAL = new Relationship.Builder()
+.name("original")
+.description("All flowfiles that are sent to Elasticsearch without
request failures go to this relationship.")
+.build();
+
+static final Relationship REL_SUCCESSFUL = new Relationship.Builder()
+.name("successful")
Review Comment:
Is there a particular reason for calling this `successful` as opposed to the
much more common `success`?
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] NIFI-12255 refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors [nifi]
ChrisSamo632 opened a new pull request, #7940: URL: https://github.com/apache/nifi/pull/7940 # Summary [NIFI-12255](https://issues.apache.org/jira/browse/NIFI-12255) refactor PutElasticsearchRecord and PutElasticsearchJson relationships to be more consistent with other processors # Tracking Please complete the following tracking steps prior to pull request creation. ### Issue Tracking - [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created ### Pull Request Tracking - [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-0` - [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-0` ### Pull Request Formatting - [x] Pull Request based on current revision of the `main` branch - [x] Pull Request refers to a feature branch with one commit containing changes # Verification Please indicate the verification steps performed prior to pull request creation. ### Build - [ ] Build completed using `mvn clean install -P contrib-check` - [ ] JDK 21 ### Licensing - ~[ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)~ - ~[ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files~ ### Documentation - ~[ ] Documentation formatting appears as expected in rendered files~ -- 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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
