osdir.com

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

Re: [Discuss] Refactoring KahaDBStore class


iMHO since you are now a commuter you have the power to call it 5,16.0. And
even make a release when you are ready.

On Thu, Nov 29, 2018 at 7:53 AM Jamie G. <jamie.goodyear@xxxxxxxxx> wrote:

> Hi Gary,
>
> Just want to clarify, you're asking to wait for 5.16.0 to be released,
> than bring in the refactoring effort?
>
> If you feel 5.16.0 is imminent than I'm happy to hold off. I'd prefer
> to get this in sooner rather than later so as to reduce the amount of
> rebasing/cherry picking i need to do in the meantime.
>
> By the way, thank you for the support and looking at the code. I
> really appreciate it :)
>
> Cheers,
> Jamie
> On Thu, Nov 29, 2018 at 8:56 AM Gary Tully <gary.tully@xxxxxxxxx> wrote:
> >
> > Hey Arthur,
> > I am not asserting that they need to be small.
> > I am pointing out that they currently are small changes; there has
> > been no significant refactor to date; it is all very conservative.
> > Release 5.16.0 as a line in the sand, then move code around to make it
> > better etc.. go for it.
> >
> > I know too well it is not perfect and I think it is great that there
> > is interest in making it better.
> >
> > On Wed, 28 Nov 2018 at 16:22, Arthur Naseef <art@xxxxxxxxxx> wrote:
> > >
> > > 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
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > >
>
-- 
Clebert Suconic