[GitHub] incubator-nifi pull request: NIFI-305: Refactoring base class from...

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-nifi pull request: NIFI-305: Refactoring base class from...

JPercivall
GitHub user gresockj opened a pull request:

    https://github.com/apache/incubator-nifi/pull/12

    NIFI-305: Refactoring base class from MergeContent to allow more binning processors

    Pulled out the binning logic from MergeContent into a superclass for binning processors.  Basically all of the min/max properties were moved to BinFiles.  Mostly focused on ensuring that the functionality was preserved from MergeContent, but there is certainly room for improvement in producing a more elegantly extensible class.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/gresockj/incubator-nifi NIFI-305

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-nifi/pull/12.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #12
   
----
commit 98afcce0dc3672fe04e2130ac72b9a201e17ba1a
Author: gresockj <[hidden email]>
Date:   2015-01-27T22:32:38Z

    NIFI-305: Refactoring superclass BinFiles from MergeContent

commit ad40903458cd2bbf98284d693a057519d8bbf775
Author: gresockj <[hidden email]>
Date:   2015-01-27T22:51:16Z

    NIFI-305: Minor documentation update

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

Re: [GitHub] incubator-nifi pull request: NIFI-305: Refactoring base class from...

Mark Payne
Joseph,


Thanks for the contribution! I’ve looked it over, and it looks really good. The only two things that I’d want to consider here, as far as I’m seeing now:


Any protected or public method we have in this new abstract class should be marked ‘final’ unless it is explicitly intended to be extended. This is to preserve the “Open for Extension, Closed or Modification” principle that we try to follow. I.e., you’re welcome to extend functionality, but you are not expected to modify its behavior. So for customValidate, for instance, we don’t want a subclass changing that behavior. So we should probably add in a hook to allow additional custom validation.


This one is really a discussion point that I’d like to bring up. I don’t know if we want to leave this new abstract class in the standard processors module. I can see this maybe moving up to the standard-services module, perhaps. But then should we be renaming it to nifi-standard-components-api instead of nifi-standard-services-api? This would give others the ability to easily use this processor for their own purposes. I wonder if we even need a new abstract class in between BinFiles and MergeContent that allows for the ability to merge the content using different Merge Formats (I.e., you can extend it and just add a new Merge Format, beyond ZIP, TAR, and FlowFile Stream. Perhaps Avro or some proprietary format).



I would really like to hear feedback on #2.


Thanks

-Mark




From: gresockj
Sent: ‎Tuesday‎, ‎January‎ ‎27‎, ‎2015 ‎6‎:‎01‎ ‎PM
To: [hidden email]





GitHub user gresockj opened a pull request:

    https://github.com/apache/incubator-nifi/pull/12

    NIFI-305: Refactoring base class from MergeContent to allow more binning processors

    Pulled out the binning logic from MergeContent into a superclass for binning processors.  Basically all of the min/max properties were moved to BinFiles.  Mostly focused on ensuring that the functionality was preserved from MergeContent, but there is certainly room for improvement in producing a more elegantly extensible class.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/gresockj/incubator-nifi NIFI-305

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-nifi/pull/12.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #12
   
----
commit 98afcce0dc3672fe04e2130ac72b9a201e17ba1a
Author: gresockj <[hidden email]>
Date:   2015-01-27T22:32:38Z

    NIFI-305: Refactoring superclass BinFiles from MergeContent

commit ad40903458cd2bbf98284d693a057519d8bbf775
Author: gresockj <[hidden email]>
Date:   2015-01-27T22:51:16Z

    NIFI-305: Minor documentation update

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

Re: [GitHub] incubator-nifi pull request: NIFI-305: Refactoring base class from...

Joe Gresock
Mark, thanks for the feedback.  Good call with the final methods -- I'll
submit another commit with those changes.  As for your second point, it
sounds like a good path forward -- I'll leave it to you to cherry-pick and
place appropriately in the project hierarchy.

On Thu, Jan 29, 2015 at 9:17 AM, Mark Payne <[hidden email]> wrote:

> Joseph,
>
>
> Thanks for the contribution! I’ve looked it over, and it looks really
> good. The only two things that I’d want to consider here, as far as I’m
> seeing now:
>
>
> Any protected or public method we have in this new abstract class should
> be marked ‘final’ unless it is explicitly intended to be extended. This is
> to preserve the “Open for Extension, Closed or Modification” principle that
> we try to follow. I.e., you’re welcome to extend functionality, but you are
> not expected to modify its behavior. So for customValidate, for instance,
> we don’t want a subclass changing that behavior. So we should probably add
> in a hook to allow additional custom validation.
>
>
> This one is really a discussion point that I’d like to bring up. I don’t
> know if we want to leave this new abstract class in the standard processors
> module. I can see this maybe moving up to the standard-services module,
> perhaps. But then should we be renaming it to nifi-standard-components-api
> instead of nifi-standard-services-api? This would give others the ability
> to easily use this processor for their own purposes. I wonder if we even
> need a new abstract class in between BinFiles and MergeContent that allows
> for the ability to merge the content using different Merge Formats (I.e.,
> you can extend it and just add a new Merge Format, beyond ZIP, TAR, and
> FlowFile Stream. Perhaps Avro or some proprietary format).
>
>
>
> I would really like to hear feedback on #2.
>
>
> Thanks
>
> -Mark
>
>
>
>
> From: gresockj
> Sent: ‎Tuesday‎, ‎January‎ ‎27‎, ‎2015 ‎6‎:‎01‎ ‎PM
> To: [hidden email]
>
>
>
>
>
> GitHub user gresockj opened a pull request:
>
>     https://github.com/apache/incubator-nifi/pull/12
>
>     NIFI-305: Refactoring base class from MergeContent to allow more
> binning processors
>
>     Pulled out the binning logic from MergeContent into a superclass for
> binning processors.  Basically all of the min/max properties were moved to
> BinFiles.  Mostly focused on ensuring that the functionality was preserved
> from MergeContent, but there is certainly room for improvement in producing
> a more elegantly extensible class.
>
> You can merge this pull request into a Git repository by running:
>
>     $ git pull https://github.com/gresockj/incubator-nifi NIFI-305
>
> Alternatively you can review and apply these changes as the patch at:
>
>     https://github.com/apache/incubator-nifi/pull/12.patch
>
> To close this pull request, make a commit to your master/trunk branch
> with (at least) the following in the commit message:
>
>     This closes #12
>
> ----
> commit 98afcce0dc3672fe04e2130ac72b9a201e17ba1a
> Author: gresockj <[hidden email]>
> Date:   2015-01-27T22:32:38Z
>
>     NIFI-305: Refactoring superclass BinFiles from MergeContent
>
> commit ad40903458cd2bbf98284d693a057519d8bbf775
> Author: gresockj <[hidden email]>
> Date:   2015-01-27T22:51:16Z
>
>     NIFI-305: Minor documentation update
>
> ----
>
>
> ---
> 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 [hidden email] or file a JIRA ticket
> with INFRA.
> ---
>



--
I know what it is to be in need, and I know what it is to have plenty.  I
have learned the secret of being content in any and every situation,
whether well fed or hungry, whether living in plenty or in want.  I can do
all this through him who gives me strength.    *-Philippians 4:12-13*
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-nifi pull request: NIFI-305: Refactoring base class from...

JPercivall
In reply to this post by JPercivall
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-nifi/pull/12


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---