nifi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Skora <jsk...@gmail.com>
Subject Re: PRs
Date Thu, 26 Nov 2015 17:49:46 GMT
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 <trkurc@gmail.com> 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 <trkurc@gmail.com> 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 <joe.witt@gmail.com> 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 <trkurc@gmail.com> 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 <trkurc@gmail.com> 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 <joe.witt@gmail.com>
> 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 <trkurc@gmail.com>
> 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 <
> >> matt.c.gilman@gmail.com>
> >> >>> > 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 <joe.witt@gmail.com>
> >> 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 <trkurc@gmail.com>
> >> >>> 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
<
> >> aldrinpiri@gmail.com
> >> >>> >
> >> >>> >> 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 <
> trkurc@gmail.com>
> >> >>> 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
> >> >>> >> >>>
> >> >>> >>
> >> >>>
> >> >>
> >> >>
> >>
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message