osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Discuss] Refactoring KahaDBStore class


Hey Gary - I agree that these changes belong on a minor version increase.

What I don't understand is the assertion that the changes between 5.15.x
and 5.16.0 need to be small.  Given that the minor version bump can mean
significant changes, as long as they are backward compatible, I see no
reason to adhere to a small set of changes between 5.15.x and 5.16.0.  Add
to that fact that ActiveMQ's minor releases are sometimes really major
changes (i.e. include breaking changes), and that makes even less sense.

Is there something more to this that perhaps I'm missing?

Making the code more maintainable by the community, as ActiveMQ is an
Apache *community* project, is the goal.  As for it being maintained for 7
years, that's great.  However, I'm sure you'll agree it's not perfect, and
community improvements are welcome.

Art


On Wed, Nov 28, 2018 at 3:30 AM Gary Tully <gary.tully@xxxxxxxxx> wrote:

> Jamie,
> you are missing my point. it is a tradeoff plain and simple. easier to
> maintain for who? It has been carefully maintained for more than 7
> years.
> Do refactoring at the beginning of a release cycle, not at the end.
> diffs between 5.15.x and 5.16 will be meaningless otherwise.
> push out 5.16.0, which has loads of incremental (non refactored) small
> changes. Then refactor away on master for 5.17.0 and make it better in
> any way that works for the future and for you.
> On Tue, 27 Nov 2018 at 15:34, Jamie G. <jamie.goodyear@xxxxxxxxx> wrote:
> >
> > Hi Gary,
> >
> > To address your concerns:
> >
> > 1. The code is being cleaned up, not completely rewritten.  This is
> > making it easier to maintain over the monolithic classes.  It's also
> > why it was brought to the community… to review it and be sure the
> > changes are just cleaning it up.  There was no intent to change the
> > logic for the reason that you suggested.
> >
> > 2. I answered above, its easy on the back port as we can just make it
> > a part of 5.15.9 (too my understanding the community supports master
> > and the last release branch).  There are little differences between 16
> > and 15.9 in the KahaDB realm.  So placing it in 15.9 does not change
> > any way it operates or works.  It only cleans up the readability of
> > the code.
> >
> >
> > "A dream you dream alone is only a dream. A dream you dream together
> > is reality."
> >
> > ― John Lennon
> >
> >
> > Cheers,
> > Jamie
> > On Tue, Nov 27, 2018 at 8:29 AM Gary Tully <gary.tully@xxxxxxxxx> wrote:
> > >
> > > Hi Jamie G,
> > > There are a few trade offs to consider:
> > >  1) those familiar with the code will have to reacquaint themselves
> > > with anything that is refactored
> > >  2) backporting fixes will be more difficult when the code structure
> changes
> > >
> > > Of the two, I think #2 is more critical.
> > >
> > > On #1:
> > > context builds up over time and code structure is an integral part of
> > > that, for better or for worse.
> > > Switching context is not something us humans like doing, most
> > > especially when stability is a key concern.
> > >
> > > Refactoring with purpose (for a new feature) can be great, doing it at
> > > this stage for readability may be less great.
> > >
> > > Mr. W. B. Yeats put it nicely: "tread softly because you tread on my
> dreams"
> > >
> > > s/dreams/mental model/
> > > On Mon, 26 Nov 2018 at 19:44, Christopher Shannon
> > > <christopher.l.shannon@xxxxxxxxx> wrote:
> > > >
> > > > I didn't say I definitely wouldn't support it but I just want to
> make sure
> > > > we are careful and the benefits of the refactor (in this case
> improved
> > > > maintainability) are really going to be worth the risk specifically
> because
> > > > of the component being touched.  Too often things get committed and
> then
> > > > issues are uncovered with the patch later that were missed.
> > > >
> > > > Some parts of the broker are critical and this is one of them
> because a bug
> > > > that corrupts a store could lead to losing lots of production data
> which is
> > > > a very different type of bug than a random web console bug or
> transport
> > > > error that just causes an error with a single client or with a single
> > > > message. Granted the risk of a critical bug being introduced with a
> > > > refactor like this is not very high but if there is one it could have
> > > > pretty bad consequences.
> > > >
> > > > Now all that being said ... as long as we are careful to make sure
> all
> > > > tests pass and have a few people thoroughly review it (such as Gary
> who has
> > > > the most experience out of anyone in KahaDB) then I would +1 the
> change for
> > > > a 5.16.0 release.
> > > >
> > > > On Mon, Nov 26, 2018 at 2:07 PM Arthur Naseef <art@xxxxxxxxxx>
> wrote:
> > > >
> > > > > Improving the existing code is a great goal.
> > > > >
> > > > > While cleaning up code is nice KahaDB has gotten pretty stable
> over the
> > > > > > years and doing a bunch of refactoring just opens it up to new
> bugs that
> > > > > > have to be fixed.  Fixing bugs is not a problem however I tend
> to be more
> > > > > > sensitive to store related changes because of the possible data
> loss or
> > > > > > corruption issues to production data that can occur from store
> bugs vs
> > > > > some
> > > > > > other random bug in the broker.
> > > > > >
> > > > >
> > > > > I understand the desire to avoid the risk of introducing bugs.
> However, as
> > > > > long as the project is active and maintained, that really isn't a
> valid
> > > > > approach to its maintenance overall.
> > > > >
> > > > > So that leads us to the question of risk mitigation.  Build-time
> tests are
> > > > > an industry standard, and ActiveMQ certainly has a large number of
> such
> > > > > tests.  Code reviews are another best-practice, and there are
> multiple
> > > > > individuals looking at these code changes now.  More are always
> welcome,
> > > > > and access is certainly not a problem!
> > > > >
> > > > > If there are specific concerns within the code changes, let's
> discuss
> > > > > them.  It'll be great to have actual technical discussions!
> > > > >
> > > > > As for the concern that this is the broker's storage, similar
> arguments
> > > > > could be made for just about any part of the code.  Reliable
> handling of
> > > > > messages is ActiveMQ's core benefit to its users.
> > > > >
> > > > >
> > > > >
> > > > > > FWIW, the current community goal is for ActiveMQ Artemis to
> become
> > > > > ActiveMQ
> > > > >
> > > > > 6.x when acceptable feature parity is reached (which is actively
> being
> > > > > > worked on).
> > > > > >
> > > > >
> > > > > Whether Artemis will eventually become ActiveMQ 6.x is not
> pertitent to
> > > > > this discussion.  Let's not open that discussion as part of this
> technical
> > > > > code conversation.
> > > > >
> > > > > Making the existing code base, which has heavy usage in the
> industry, more
> > > > > maintainable is always a good goal to achieve.  And having more
> folks in
> > > > > the community engaging in working on the project can only benefit
> the
> > > > > entire community regardless of the long-term destination.
> > > > >
> > > > > Art
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Mon, Nov 26, 2018 at 10:22 AM Justin Bertram <
> jbertram@xxxxxxxxxx>
> > > > > wrote:
> > > > >
> > > > > > FWIW, the current community goal is for ActiveMQ Artemis to
> become
> > > > > ActiveMQ
> > > > > > 6.x when acceptable feature parity is reached (which is actively
> being
> > > > > > worked on).
> > > > > >
> > > > > >
> > > > > > Justin
> > > > > >
> > > > > >
> > > > > > On Mon, Nov 26, 2018 at 11:01 AM Jamie G. <
> jamie.goodyear@xxxxxxxxx>
> > > > > > wrote:
> > > > > >
> > > > > > > The idea here is to ensure that it’s development and
> maintenance
> > > > > > > moving forward is easier as we work to make it better in the
> future.
> > > > > > >
> > > > > > > KahaDB currently has massive classes (KahaDBStore,
> MessageDatabase)
> > > > > > > and code base and is extremely hard to follow.  My desire to
> do this
> > > > > > > was to make this so we could make the continued maintenance
> easier and
> > > > > > > also make it more inviting to improvements.  The unit tests
> all pass,
> > > > > > > so as long as we have a good solid testing coverage, the risks
> are not
> > > > > > > huge.  If a bug appears to be introduced, than we may have
> uncovered a
> > > > > > > testing hole - finding these is a good thing.
> > > > > > >
> > > > > > > Since we are going to continue to support ActiveMQ moving
> forward,
> > > > > > > it’s a good idea to make it more maintainable.  My motivation
> to doing
> > > > > > > this was spurred by the recent JIRAs surrounding KahaDB I
> helped out
> > > > > > > upon.  I really believe this is a good improvement to help
> moving
> > > > > > > ActiveMQ forward.
> > > > > > > On Mon, Nov 26, 2018 at 12:33 PM Christopher Shannon
> > > > > > > <christopher.l.shannon@xxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > I'm not really sure this is worthwhile or something we want
> to do...I
> > > > > > > would
> > > > > > > > have to think about it more before I gave it a +1.
> > > > > > > >
> > > > > > > > While cleaning up code is nice KahaDB has gotten pretty
> stable over
> > > > > the
> > > > > > > > years and doing a bunch of refactoring just opens it up to
> new bugs
> > > > > > that
> > > > > > > > have to be fixed.  Fixing bugs is not a problem however I
> tend to be
> > > > > > more
> > > > > > > > sensitive to store related changes because of the possible
> data loss
> > > > > or
> > > > > > > > corruption issues to production data that can occur from
> store bugs
> > > > > vs
> > > > > > > some
> > > > > > > > other random bug in the broker.
> > > > > > > >
> > > > > > > > On Sun, Nov 25, 2018 at 11:59 PM Jean-Baptiste Onofré <
> > > > > jb@xxxxxxxxxxxx
> > > > > > >
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > OK, got it. It's more a syntax/codebase organization
> refactoring.
> > > > > > > > >
> > > > > > > > > If there's no impact on the behavior and features, +1 from
> my side.
> > > > > > > > >
> > > > > > > > > Regards
> > > > > > > > > JB
> > > > > > > > >
> > > > > > > > > On 25/11/2018 21:21, Jamie G. wrote:
> > > > > > > > > > Initially its to make KahaDB classes easier to read &
> maintain.
> > > > > > > > > > Eventually it will help in features/performance; smaller
> classes
> > > > > > are
> > > > > > > > > > easier to grok, easier to see improvements.
> > > > > > > > > >
> > > > > > > > > > Instead of trying to refactor all of it in one go, I'm
> taking the
> > > > > > > > > > approach of one area at a time.
> > > > > > > > > >
> > > > > > > > > > One pass for breaking out objects.
> > > > > > > > > > Another pass for small functional improvements.
> > > > > > > > > > Perhaps future passes for new Java features (bring all
> code up to
> > > > > > > Java
> > > > > > > > > > 8 perhaps?).
> > > > > > > > > >
> > > > > > > > > > On Sun, Nov 25, 2018 at 4:32 PM Jean-Baptiste Onofré <
> > > > > > > jb@xxxxxxxxxxxx>
> > > > > > > > > wrote:
> > > > > > > > > >>
> > > > > > > > > >> Hi Jamie,
> > > > > > > > > >>
> > > > > > > > > >> That's interesting.
> > > > > > > > > >>
> > > > > > > > > >> What's the rationale behind the refactoring ? New
> features or
> > > > > perf
> > > > > > > > > >> improvements ?
> > > > > > > > > >>
> > > > > > > > > >> Regards
> > > > > > > > > >> JB
> > > > > > > > > >>
> > > > > > > > > >> On 25/11/2018 20:16, Jamie G. wrote:
> > > > > > > > > >>> Hi All,
> > > > > > > > > >>>
> > > > > > > > > >>> I've taken some time to prototype a refactored
> KahaDBStore
> > > > > class:
> > > > > > > > > >>>
> https://github.com/jgoodyear/activemq/tree/KahaDBRefactor
> > > > > > > > > >>>
> > > > > > > > > >>> As KahaDBStore exists in Master, it contains 7 internal
> > > > > classes,
> > > > > > > over
> > > > > > > > > >>> some 1677 lines of code.
> > > > > > > > > >>>
> > > > > > > > > >>> In my refactor branch I've separated out those classes
> into
> > > > > their
> > > > > > > own
> > > > > > > > > >>> files, and applied some gentle clean code practices to
> help
> > > > > make
> > > > > > > these
> > > > > > > > > >>> files easier to read and maintain.
> > > > > > > > > >>>
> > > > > > > > > >>> I'd like to gather feed back from the community; I've
> taken
> > > > > care
> > > > > > to
> > > > > > > > > >>> change functionality as little as possible - the aim
> here is to
> > > > > > > reduce
> > > > > > > > > >>> complexity and improve maintainability. If the
> community feels
> > > > > > > this is
> > > > > > > > > >>> a worth while goal than I'll open a card on Jira &
> prepare a
> > > > > PR.
> > > > > > > > > >>>
> > > > > > > > > >>> Notes:
> > > > > > > > > >>> ActiveMQ KahaDB Store  and ActiveMQ-Unit-Tests suites
> remain
> > > > > > > passing
> > > > > > > > > >>> after refactor.
> > > > > > > > > >>>
> > > > > > > > > >>> Cheers,
> > > > > > > > > >>> Jamie
> > > > > > > > > >>>
> > > > > > > > > >>
> > > > > > > > > >> --
> > > > > > > > > >> Jean-Baptiste Onofré
> > > > > > > > > >> jbonofre@xxxxxxxxxx
> > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > jbonofre@xxxxxxxxxx
> > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > Talend - http://www.talend.com
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
>