[GitHub] nifi issue #2796: NIFI-5275 PostHTTP SocketConfig setup, fixed connection po...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2796 +1. Looks good, and we've been running the new version successfully for weeks. ---
[GitHub] nifi issue #2416: NIFI 4774: Provide alternate implementation of Write-Ahead...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2416 @markap14 yes... under this somewhat unusual circumstance, my proposed solution would sacrifice data for consistency. However, if alwaysSync is set false, there isn't a guarantee of no loss anyway. So, we'd really be no worse off than we are, except that the plug getting kicked out of the wall wouldn't kill your repo. Also, I fully agree that an implementation that DOES guarantee no loss of CREATEs would be for the best. My proposal above doesn't address that in any way. It lives inside the "all or nothing" guarantee of the current (/previous?) WriteAheadFFR. ---
[GitHub] nifi issue #2416: NIFI 4774: Provide alternate implementation of Write-Ahead...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2416 Again, I would submit that it isn't violating the data loss guarantees of the original implementation. Yes, some data could potentially be lost, but none that it guaranteed to keep, and in doing so it prevents corruption... in a handful of easily understandable lines, that don't fundamentally alter the implementation. I want to be clear... I'm in no way opposed to improvements. However, for operational data flows there is going to be some trepidation about large changes to such a key component... especially one that we've had previous issues with. The smaller / more incremental the changes, and the more time allowed for evaluation under load, the more comfortable I would imagine people would be. ---
[GitHub] nifi issue #2416: NIFI 4774: Provide alternate implementation of Write-Ahead...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2416 I'm glad there's support for making this opt in. One point on @joewitt 's comment : "The claim of a simple fix being available to close the previous gaps doesn't appear to be backed with a suggested implementation though it does look like you received a good response to why that wasn't feasible." ... the reasons given were reasons why @markap14 's original proposed solution wouldn't work, not why another solution might not. For example (as suggested previously in email): > the immediate fix that I see needing is that when building the transaction map[1], we need to keep track of the *lowest* encountered corrupt transaction. Then, when iterating over the transaction map to rebuild the repo, we need to stop when that transaction is reached... after that we can't trust that there are no missing transactions which could lead to repo corruption or incorrect state. This is the relatively simple fix that I believe would be more appropriate to the WhriteAheadFlowFileRepository, as it would only prevent a known bug from causing corruption, as opposed to a major rewrite. I never got any response suggesting this wouldn't work, rather simply that another solution was preferred... which turned out not to be feasible. At that point, instead of circling back and trying to fix the bug, it appears as though the rewrite began. If there's a reason the author of the original implementation doesn't believe my suggestion would work to prevent the observed corruption, I'd be happy to take another look. [1] https://github.com/apache/nifi/blob/0f2ac39f69c1a744f151f0d924c9978f6790b7f7/nifi-commons/nifi-write-ahead-log/src/main/java/org/wali/MinimalLockingWriteAheadLog.java#L444 ---
[GitHub] nifi issue #2416: NIFI 4774: Provide alternate implementation of Write-Ahead...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2416 I'll grant NIFI-4775 may raise issues with my proposed solution. However, there is a problem right now. My proposed solution addresses the problem right now. Future modification may require adjustments to previous assumptions. That, however, is a problem for the future. In any case, after doing some experimentation, I'm not sure the current version of NIFI-4775 is the correct approach. And whatever the eventual approach is, it may more appropriately be a new implementation (as discussed above). I don't think we should put off correcting current bugs because they may complicate potential future features. ---
[GitHub] nifi issue #2416: NIFI 4774: Provide alternate implementation of Write-Ahead...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2416 So, I was under the impression this was still a WIP. I am a HUGE -1 on this change. As @markap14 stated above, this is a critical section of code. And while the previous version has serious flaws, they are at least somewhat know based on a long period of use. In other words, we know there are problems, but they only bite us every so often. This major rewrite of a critical piece being essentially forced on users, likely without their knowledge unless they are paying close attention, seems less than ideal. This new implementation will need SIGNIFICANT testing before it can be trusted to the same degree as the previous, even with its issues. I would have greatly preferred, and will still advocate for, making this a new implementation vs. a change to the WriteAheadFlowFileRepository (e.g. SequentialWriteAheadFlowFileRepository). Again, there are other issues, but avoiding repo corruption in the previous WriteAheadFlowFileRepository would have been a reasonably simple fix, not requiring this rewrite. While the rewrite may have other benefits, making it a new implementation (even if you were to make it the default...) would give users the opportunity to evaluate and decide for themselves when they are ready to move to the new repo, without forcing them to postpone an upgrade to 1.6.0 which has other worthwhile changes. I know critical sections are change all the time, and that users won't always be aware of the changes. However, the criticality of this section combined with it's history means I think we should tread a little more lightly. ---
[GitHub] nifi pull request #2458: NIFI-2630 Allow PublishJMS to send TextMessages
Github user devriesb commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2458#discussion_r167252827
--- Diff:
nifi-nar-bundles/nifi-jms-bundle/nifi-jms-processors/src/main/java/org/apache/nifi/jms/processors/PublishJMS.java
---
@@ -131,4 +143,10 @@ protected JMSPublisher
finishBuildingJmsWorker(CachingConnectionFactory connecti
session.read(flowFile, in -> StreamUtils.fillBuffer(in,
messageContent, true));
return messageContent;
}
+
+private String extractTextMessageBody(FlowFile flowFile,
ProcessSession session) {
+final StringWriter writer = new StringWriter();
+session.read(flowFile, in -> IOUtils.copy(in, writer,
Charset.defaultCharset()));
--- End diff --
i'm not sure if this is a helpful "middle ground" suggestion or not, but
you could add a charset property that itself defaults to
Charset.defaultCharset(). That way the out of the box behavior uses the
charset specified for the jvm, but can be overridden on a per processor basis
if needed.
---
[GitHub] nifi issue #2461: NIFI-4856 Removed deprecated ByteArrayInputStream referenc...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2461 Agreed on specifying a char set, but other than that I think it looks good! ---
[GitHub] nifi issue #2416: [WIP] NIFI 4774: Provide alternate implementation of Write...
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2416 while i in no way object to a new implementation, I'm not sure that is the correct solution to the bug described in NIFI-4774[1]. A new implementation would need to be tested to a degree that a tweak to the existing implementation would not, and fixing this bug in a timely fashion would seem to be a worthy goal. [1] https://issues.apache.org/jira/browse/NIFI-4774 ---
[GitHub] nifi issue #2284: NIFI-4504, NIFI-4505 added methods to MapCache API …
Github user devriesb commented on the issue: https://github.com/apache/nifi/pull/2284 Koji, "Are all of these three methods, removeAndGet, removeByPatternAndGet and keySet required by the folks you know of?" - Yes. "atomicity is not that important in these method IMHO" - I think you'll find not everyone shares that opinion. Ultimately, removeAndGet behaves the same as the remove() method of the java Map interface, which is the behavior I (and I would suspect many others) would expect. While I understand providing a "flavor" of this method that does not return the removed value to save network traffic, I would have preferred those be named differently. However, since we're now locked into "remove" not returning the removed value, "removeAndGet" is likely as good as we're going to get. Brandon On Wed, Dec 27, 2017 at 7:59 PM Koji Kawamura wrote: > @mosermw <https://github.com/mosermw> Thank you for updating the PR, it's > much easier to review now. The code implemented at > DistributedMapCacheClientService and MapCacheServer looks good to me. > Travis error should be fine as the failure looks depending on execution > condition. > > However, let me ask one more time since adding methods is easy but hard to > remove afterwards for things like this. Are all of these three methods, > removeAndGet, removeByPatternAndGet and keySet required by the folks you > know of? I prefer to minimize the addition as those methods are only > supported by DistributedMapCacheClientService, which means those will not > be used by most developers who write Processors. Also, it confuses such > developers which to choose from remove and removeAndGet. Probably I > imagine these concerns are what made you struggle with implementing default > method to just throw UnsupportedOperationException. > > If there is no significant needs for removeAndGet and > removeByPatternAndGet, then I'd prefer omit these method from > DistributedMapCacheClient interface. As long as we support keySet, pretty > much everything can be done at the caller side. The only benefit to have > AndGet methods I am aware of is reducing the network traffic, and atomicity > (atomicity is not that important in these method IMHO). Do you think those > are more important than adding mostly unsupported methods into a common > interface? > > â > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/nifi/pull/2284#issuecomment-354207212>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AB2qyBEO72vOhV9p-2HcFIUoAS3_UM9xks5tEufigaJpZM4Qmhur> > . > ---
[GitHub] nifi pull request #1202: NIFI-2854: Refactor repositories and swap files to ...
Github user devriesb commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1202#discussion_r87807081
--- Diff:
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/stream/io/BufferedInputStream.java
---
@@ -16,19 +16,445 @@
*/
package org.apache.nifi.stream.io;
+import java.io.IOException;
import java.io.InputStream;
/**
* This class is a slight modification of the BufferedInputStream in the
java.io package. The modification is that this implementation does not provide
synchronization on method calls, which means
* that this class is not suitable for use by multiple threads. However,
the absence of these synchronized blocks results in potentially much better
performance.
*/
-public class BufferedInputStream extends java.io.BufferedInputStream {
+public class BufferedInputStream extends InputStream {
--- End diff --
I agree with Joe... we've had instance where eclipse (and I assume IntelliJ
/ other IDEs) suggest the nifi version of BufferedInputStream, resulting in
bad, unexpected behavior. The name is really unfortunate. Perhaps move the
functionality to "UnsycnchronizedBufferedInputStream", modify nifi's
BufferedInputStream to be an empty extension, and deprecate it. Then complete
the rename / remove in a future release (like a major version bump, if the
breaking change is the concern...).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
