I gave it a quick read through and I think it looks good. It looks like the properties are added to all the processors and the endpoint and http client changes are set in AbstractAwsProcessor.
But that was a quick read because we're about to head out the door, I will look at it in depth tonight. On Thu, Nov 26, 2015 at 12:31 PM, Tony Kurc <[email protected]> wrote: > I created NIFI-1225 which are changes to S3 processors without Multipart. > Submitted a patch for review. Joe Skora, would you have a chance to look it > over? > > On Thu, Nov 26, 2015 at 12:41 AM, Tony Kurc <[email protected]> wrote: > > > I'll second what joe said. non-trivial and a less than ideal API to work > > with. seriously?! no expiry! > > > > > > > > On Thu, Nov 26, 2015 at 12:17 AM, Joe Witt <[email protected]> wrote: > > > >> Understood tony - thanks for digging into the review so thoroughly and > >> Joe thank you. This is a very non-trivial contrib. > >> > >> On Thu, Nov 26, 2015 at 12:12 AM, Tony Kurc <[email protected]> wrote: > >> > I recommend we push NIFI-1107 to next release. We discovered some > unfun > >> > issues the S3 Multipart "API" creates, notably, leaving dangling > pieces > >> > around [1]: > >> > > >> > "Once you initiate a multipart upload there is no expiry; you must > >> > explicitly complete or abort the multipart upload" > >> > > >> > And charging while they're there [2]: > >> > > >> > "After you initiate multipart upload and upload one or more parts, you > >> must > >> > either complete or abort multipart upload in order to stop getting > >> charged > >> > for storage of the uploaded parts. Only after you either complete or > >> abort > >> > multipart upload, Amazon S3 frees up the parts storage and stops > >> charging > >> > you for the parts storage." > >> > > >> > This, plus grappling with persisting state for the multipart (and lack > >> of > >> > framework support for persistent state) means I think we have some > more > >> > work to do despite the feature being in a "works" state. > >> > > >> > [1] > >> http://docs.aws.amazon.com/AmazonS3/latest/dev/uploadobjusingmpu.html > >> > [2] http://docs.aws.amazon.com/AmazonS3/latest/dev/mpuoverview.html > >> > > >> > On Wed, Nov 25, 2015 at 7:16 PM, Tony Kurc <[email protected]> wrote: > >> > > >> >> Things that make me feel better: The persistence mechanism is very > >> similar > >> >> to that of ListHDFS. > >> >> > >> >> > >> >> > >> > https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-hadoop-bundle/nifi-hdfs-processors/src/main/java/org/apache/nifi/processors/hadoop/ListHDFS.java#L417 > >> >> > >> >> On Tue, Nov 24, 2015 at 10:56 PM, Joe Witt <[email protected]> > wrote: > >> >> > >> >>> Tags are a great place to mark experimental. We used to plan for > this > >> >>> concept outright and make it look at scary and such on the ui. But > >> >>> folks just didn't care. They used it anyway. Happy to revisit it > but > >> >>> for now perhaps just adding a tag of experimental is enough. > >> >>> > >> >>> If the existing code path is largely untouched then that is > certainly > >> >>> great for moving the ball forward. In fairness to Joe S or anyone > >> >>> that has to persist internal process state until we offer that as > part > >> >>> of the framework it is much harder than we want it to be for people. > >> >>> Will take a look through the state methods but Payne is probably the > >> >>> best at playing the wack a mole edge case game for such things. > >> >>> > >> >>> On Tue, Nov 24, 2015 at 10:52 PM, Tony Kurc <[email protected]> > wrote: > >> >>> > So, I beat on the the patch for NIFI-1107, and as I suspected, it > is > >> >>> > awfully low risk for existing flows, but I think I'd need a second > >> >>> opinion > >> >>> > on how state is kept for resuming uploads. I believe it will work, > >> and > >> >>> it > >> >>> > looks like a lot of the edge cases are covered if somehow state is > >> lost > >> >>> or > >> >>> > corrupted, but I'm not sure if I am comfortable with how it fits > >> >>> > architecturally. If someone has cycles, and can peruse the *State > >> >>> methods > >> >>> > (getState, persistState, ...) and weigh in, it would accelerate my > >> >>> review > >> >>> > significantly. > >> >>> > > >> >>> > Also, it sure would be great to mark features as experimental! > >> >>> > > >> >>> > > >> >>> > > >> >>> > On Tue, Nov 24, 2015 at 10:36 PM, Matt Gilman < > >> [email protected]> > >> >>> > wrote: > >> >>> > > >> >>> >> These tickets [1][2] address the incorrect validation errors we > >> were > >> >>> >> seeing for processors that include the Input Required annotation. > >> These > >> >>> >> were bugs that slipped through the NIFI-810 the review. Would be > >> good > >> >>> to > >> >>> >> include if possible but I understand we need to draw the line > >> >>> somewhere. > >> >>> >> > >> >>> >> As for NIFI-655, I've been struggling getting an LDAP server > stood > >> up > >> >>> that > >> >>> >> uses 2 way SSL. Hopefully we can get that squared away soon and > >> wrap > >> >>> this > >> >>> >> one up. :) > >> >>> >> > >> >>> >> Matt > >> >>> >> > >> >>> >> [1] https://issues.apache.org/jira/browse/NIFI-1198 > >> >>> >> [2] https://issues.apache.org/jira/browse/NIFI-1203 > >> >>> >> > >> >>> >> Sent from my iPhone > >> >>> >> > >> >>> >> > On Nov 24, 2015, at 10:23 PM, Joe Witt <[email protected]> > >> wrote: > >> >>> >> > > >> >>> >> > Given the testing to NIFI-1192 and review of NIFI-631 done > >> already > >> >>> >> > both are lower risk I think. > >> >>> >> > > >> >>> >> > NIFI-1107 seems very useful and helpful but we do need to be > >> careful > >> >>> >> > given that we know this one is already in use and this is a > >> >>> >> > substantive change. > >> >>> >> > > >> >>> >> > If there are folks that can dig into review/testing of > NIFI-1107 > >> that > >> >>> >> > would be great. Waiting for word on NIFI-655 readiness then I > >> think > >> >>> >> > we should go cold and just focus on testing an RC. > >> >>> >> > > >> >>> >> > Thanks > >> >>> >> > Joe > >> >>> >> > > >> >>> >> >> On Tue, Nov 24, 2015 at 4:22 PM, Tony Kurc <[email protected]> > >> >>> wrote: > >> >>> >> >> Agreed. I know there has already been a good deal of > discussion > >> >>> about > >> >>> >> >> design on all these. > >> >>> >> >> > >> >>> >> >>> On Tue, Nov 24, 2015 at 4:14 PM, Aldrin Piri < > >> [email protected] > >> >>> > > >> >>> >> wrote: > >> >>> >> >>> > >> >>> >> >>> No qualms here. If they look good to go while the work and > >> testing > >> >>> >> >>> surrounding NIFI-655 wraps up, they might as well be > included. > >> >>> Would > >> >>> >> not > >> >>> >> >>> want to delay the release should any of these become > >> protracted in > >> >>> >> terms of > >> >>> >> >>> iterations. > >> >>> >> >>> > >> >>> >> >>>> On Tue, Nov 24, 2015 at 4:05 PM, Tony Kurc < > [email protected]> > >> >>> wrote: > >> >>> >> >>>> > >> >>> >> >>>> All, > >> >>> >> >>>> I was reviewing github PRs and wondering whether anyone > >> objected > >> >>> to > >> >>> >> >>>> slipping a couple that look like they're very close into > >> 0.4.0. > >> >>> >> >>>> > >> >>> >> >>>> NIFI-1192 (#131) > >> >>> >> >>>> NIFI-631 (#113) > >> >>> >> >>>> NIFI-1107 (#192) > >> >>> >> >>>> > >> >>> >> >>>> I should have some review cycles tonight. Lots of comments > on > >> them > >> >>> >> all, > >> >>> >> >>> and > >> >>> >> >>>> have good "momentum". > >> >>> >> >>>> > >> >>> >> >>>> Tony > >> >>> >> >>> > >> >>> >> > >> >>> > >> >> > >> >> > >> > > > > >
