Re: [PR] NIFI-12986 Tidy up JavaDoc of ProcessSession [nifi]
exceptionfactory closed pull request #8620: NIFI-12986 Tidy up JavaDoc of ProcessSession URL: https://github.com/apache/nifi/pull/8620 -- 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-12986 Tidy up JavaDoc of ProcessSession [nifi]
exceptionfactory commented on PR #8620: URL: https://github.com/apache/nifi/pull/8620#issuecomment-2052116435 Thanks for taking the time to research and evaluate current approaches @EndzeitBegins. I believe this ultimately goes back to the roots of HTML where the `` is not required in most cases, since HTML is not a subset of XML, requiring properly closed elements. I lean in the direction of clarity, including the closing tag, but given that fact that it is valid without, and the example from the Oracle documentation, I am fine with leaving it as it stands, without the closing tag so we can proceed. Thanks again for the thoughtful interaction. -- 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-12986 Tidy up JavaDoc of ProcessSession [nifi]
EndzeitBegins commented on PR #8620: URL: https://github.com/apache/nifi/pull/8620#issuecomment-2052096719 To be honest I was (and still am really) somewhat confused regarding the paragraphs. I've seen both styles applied throughout the NiFi codebase, e.g. [here](https://github.com/apache/nifi/blob/cc7af91f97dd1844cc6c8d30bd47d6865cb20aaa/nifi-api/src/main/java/org/apache/nifi/time/DurationFormat.java#L62-L85) vs. [here](https://github.com/apache/nifi/blob/62a4ece55d08d5488e42819f27bbecd864e9e7dc/nifi-api/src/main/java/org/apache/nifi/reporting/ReportingTask.java#L24-L58). A web search didn't help that much either as it yielded ambiguous results. This [article from Oracle](https://www.oracle.com/de/technical-resources/articles/java/javadoc-tool.html#format) uses single `` notation. Others suggested that that wouldn't be valid due to the closing tag missing. I don't have hard feelings about that. 路♂️ I just took the less verbose option. Regardless of what we choose to use, it may be a good idea to unify on one declaration style inside NiFi, not in this PR of course, to avoid that confusion in the future. Do you have a verdict on what style should be preferred for NiFi @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-12986 Tidy up JavaDoc of ProcessSession [nifi]
EndzeitBegins commented on PR #8620: URL: https://github.com/apache/nifi/pull/8620#issuecomment-2050252510 Makes perfect sense @exceptionfactory. I removed the default implementations for now and might open a separate PR once the documentation adjustments are merged. -- 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-12986 Tidy up JavaDoc of ProcessSession [nifi]
exceptionfactory commented on PR #8620: URL: https://github.com/apache/nifi/pull/8620#issuecomment-2050053994 Thanks for the helpful reply @EndzeitBegins. I evaluated the current method implementations that align with the proposed default implementations, and it seems like introducing the new default implementation methods could be helpful. However, it probably means that the existing implementations should be removed to avoid confusion. From that perspective, it would be better to consider those changes as a separate PR, keeping this one focused solely on documentation updates. -- 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-12986 Tidy up JavaDoc of ProcessSession [nifi]
EndzeitBegins commented on PR #8620: URL: https://github.com/apache/nifi/pull/8620#issuecomment-2048209586 Thank you for taking a look @exceptionfactory. I know this is a large change in terms of lines changes, so I really appreciate that. While going through the interface declaration, I noticed that those functions are just a shorthand overload of their respective functions with more parameters. The behaviour of calling `commitAsync()` is defined equal to calling `commitAsync(Runnable onSuccess, Consumer onFailure)` with both parameters set to `null`, as both parameters are optional. In fact, the same applies to the "sibling" method `commitAsync(Runnable onSuccess)` which has an existing default implementation of `commitAsync(onSuccess, null);`. I figured adding a default for `commitAsync()` would further align both function overloads. A similar reasoning can be applied to `merge(Collection sources, FlowFile destination)` who's behavior is defined akin to that of `merge(Collection sources, FlowFile destination, byte[] header, byte[] footer, byte[] demarcator)` when passing `null` for all of `header`, `footer`, and `demarcator`. However, this time there is no other sibling overload with a default. As adding a default implementation is not a breaking change nor alters the behavior for any existing implementation I figured that's a sensible addition. However, you're right that it's out of scope of the original issue description. With this background, what are your thoughts on this? Should I revert the addition of those default implementations or do you follow the reasoning above? -- 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-12986 Tidy up JavaDoc of ProcessSession [nifi]
exceptionfactory commented on code in PR #8620:
URL: https://github.com/apache/nifi/pull/8620#discussion_r1558294655
##
nifi-api/src/main/java/org/apache/nifi/processor/ProcessSession.java:
##
@@ -173,846 +120,783 @@ public interface ProcessSession {
* getDataFromSource();
* session.commitAsync( () -> acknowledgeReceiptOfData() );
*
+ *
+ * If the session cannot be committed, an error will be logged and the
session will be rolled back instead.
*
- * @throws IllegalStateException if called from within a callback (See
{@link #write(FlowFile, StreamCallback)}, {@link #write(FlowFile,
OutputStreamCallback)},
- * {@link #read(FlowFile, InputStreamCallback)}).
+ * @throws IllegalStateException if detected that this method is being
called from within a read or write callback
+ * (see {@link #read(FlowFile, InputStreamCallback)}, {@link
#write(FlowFile, StreamCallback)},
+ * {@link #write(FlowFile, OutputStreamCallback)}) or while a
read or write stream is open
+ * (see {@link #read(FlowFile)}, {@link #write(FlowFile)}).
+ * @throws FlowFileHandlingException if not all {@link FlowFile}s acted
upon within this session are accounted for
+ * such that they have a transfer identified or where marked
for removal. Automated rollback occurs.
+ */
+default void commitAsync() {
Review Comment:
This appears to be a new method.
##
nifi-api/src/main/java/org/apache/nifi/processor/ProcessSession.java:
##
@@ -173,846 +120,783 @@ public interface ProcessSession {
* getDataFromSource();
* session.commitAsync( () -> acknowledgeReceiptOfData() );
*
+ *
+ * If the session cannot be committed, an error will be logged and the
session will be rolled back instead.
*
- * @throws IllegalStateException if called from within a callback (See
{@link #write(FlowFile, StreamCallback)}, {@link #write(FlowFile,
OutputStreamCallback)},
- * {@link #read(FlowFile, InputStreamCallback)}).
+ * @throws IllegalStateException if detected that this method is being
called from within a read or write callback
+ * (see {@link #read(FlowFile, InputStreamCallback)}, {@link
#write(FlowFile, StreamCallback)},
+ * {@link #write(FlowFile, OutputStreamCallback)}) or while a
read or write stream is open
+ * (see {@link #read(FlowFile)}, {@link #write(FlowFile)}).
+ * @throws FlowFileHandlingException if not all {@link FlowFile}s acted
upon within this session are accounted for
+ * such that they have a transfer identified or where marked
for removal. Automated rollback occurs.
+ */
+default void commitAsync() {
+commitAsync(null, null);
+}
+
+/**
+ * Commits the current session ensuring all operations against {@link
FlowFile}s within this session are atomically persisted.
+ * All FlowFiles operated on within this session must be accounted for by
transfer or removal or the commit will fail.
+ *
+ * If the session is successfully committed, the given {@code onSuccess}
{@link Runnable} will be called.
+ * At the point that the session commit is completed, any calls to {@link
#rollback()} / {@link #rollback(boolean)}
+ * will not undo that session commit but instead roll back any changes
that may have occurred since.
+ *
+ * If, for any reason, the session could not be committed, an error-level
log message will be generated,
+ * but the caller will not have a chance to perform any cleanup logic.
+ * If such logic is necessary, use {@link #commitAsync(Runnable,
Consumer)} instead.
+ *
+ * Unlike the {@link #commit()} method, the persistence of data to the
repositories
+ * is not guaranteed to have occurred by the time that this method returns.
*
- * @throws FlowFileHandlingException if any FlowFile is not appropriately
accounted for by transferring it to a Relationship (see {@link
#transfer(FlowFile, Relationship)})
- * or removed (see {@link #remove(FlowFile)}.
+ * @param onSuccess {@link Runnable} that will be called if and when the
session is successfully committed; may be null
+ * @throws IllegalStateException if detected that this method is being
called from within a read or write callback
+ * (see {@link #read(FlowFile, InputStreamCallback)}, {@link
#write(FlowFile, StreamCallback)},
+ * {@link #write(FlowFile, OutputStreamCallback)}) or while a
read or write stream is open
+ * (see {@link #read(FlowFile)}, {@link #write(FlowFile)}).
+ * @throws FlowFileHandlingException if not all {@link FlowFile}s acted
upon within this session are accounted for
+ * such that they have a transfer identified or where marked
for removal. Automated rollback occurs.
*/
default void
[PR] NIFI-12986 Tidy up JavaDoc of ProcessSession [nifi]
EndzeitBegins opened a new pull request, #8620: URL: https://github.com/apache/nifi/pull/8620 # Summary Adjusts JavaDoc of `ProcessSession` to fix some minor typos and and align documentation drifts between method overloads, in order to make the JavaDoc more consistent. [NIFI-12986](https://issues.apache.org/jira/browse/NIFI-12986) # 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 - [x] 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) - [x] New dependencies are documented in applicable `LICENSE` and `NOTICE` files ### Documentation - [x] 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
