activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Tully <gary.tu...@gmail.com>
Subject Re: [Discuss] Refactoring KahaDBStore class
Date Thu, 29 Nov 2018 12:26:32 GMT
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@amlinv.com> 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@gmail.com> 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@gmail.com> 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@gmail.com> 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@gmail.com> 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@amlinv.com>
> > 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@apache.org>
> > > > > > 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@gmail.com>
> > > > > > > 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@gmail.com> 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@nanthrax.net
> > > > > > > >
> > > > > > > > > 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@nanthrax.net>
> > > > > > > > > > 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@apache.org
> > > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > > jbonofre@apache.org
> > > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > > Talend - http://www.talend.com
> > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> >

Mime
View raw message