[Discuss] Refactoring KahaDBStore class

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

[Discuss] Refactoring KahaDBStore class

jgoodyear
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
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

Jean-Baptiste Onofré
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é
[hidden email]
http://blog.nanthrax.net
Talend - http://www.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

jgoodyear
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é <[hidden email]> 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é
> [hidden email]
> http://blog.nanthrax.net
> Talend - http://www.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

Jean-Baptiste Onofré
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é <[hidden email]> 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é
>> [hidden email]
>> http://blog.nanthrax.net
>> Talend - http://www.talend.com

--
Jean-Baptiste Onofré
[hidden email]
http://blog.nanthrax.net
Talend - http://www.talend.com
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

christopher.l.shannon
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é <[hidden email]>
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é <[hidden email]>
> 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é
> >> [hidden email]
> >> http://blog.nanthrax.net
> >> Talend - http://www.talend.com
>
> --
> Jean-Baptiste Onofré
> [hidden email]
> http://blog.nanthrax.net
> Talend - http://www.talend.com
>
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

jgoodyear
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
<[hidden email]> 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é <[hidden email]>
> 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é <[hidden email]>
> > 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é
> > >> [hidden email]
> > >> http://blog.nanthrax.net
> > >> Talend - http://www.talend.com
> >
> > --
> > Jean-Baptiste Onofré
> > [hidden email]
> > http://blog.nanthrax.net
> > Talend - http://www.talend.com
> >
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

jbertram
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. <[hidden email]> 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
> <[hidden email]> 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é <[hidden email]>
> > 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é <
> [hidden email]>
> > > 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é
> > > >> [hidden email]
> > > >> http://blog.nanthrax.net
> > > >> Talend - http://www.talend.com
> > >
> > > --
> > > Jean-Baptiste Onofré
> > > [hidden email]
> > > http://blog.nanthrax.net
> > > Talend - http://www.talend.com
> > >
>
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

artnaseef
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 <[hidden email]> 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. <[hidden email]>
> 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
> > <[hidden email]> 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é <[hidden email]
> >
> > > 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é <
> > [hidden email]>
> > > > 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é
> > > > >> [hidden email]
> > > > >> http://blog.nanthrax.net
> > > > >> Talend - http://www.talend.com
> > > >
> > > > --
> > > > Jean-Baptiste Onofré
> > > > [hidden email]
> > > > http://blog.nanthrax.net
> > > > Talend - http://www.talend.com
> > > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

christopher.l.shannon
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 <[hidden email]> 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 <[hidden email]>
> 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. <[hidden email]>
> > 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
> > > <[hidden email]> 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é <
> [hidden email]
> > >
> > > > 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é <
> > > [hidden email]>
> > > > > 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é
> > > > > >> [hidden email]
> > > > > >> http://blog.nanthrax.net
> > > > > >> Talend - http://www.talend.com
> > > > >
> > > > > --
> > > > > Jean-Baptiste Onofré
> > > > > [hidden email]
> > > > > http://blog.nanthrax.net
> > > > > Talend - http://www.talend.com
> > > > >
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

gtully
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
<[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> > 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. <[hidden email]>
> > > 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
> > > > <[hidden email]> 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é <
> > [hidden email]
> > > >
> > > > > 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é <
> > > > [hidden email]>
> > > > > > 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é
> > > > > > >> [hidden email]
> > > > > > >> http://blog.nanthrax.net
> > > > > > >> Talend - http://www.talend.com
> > > > > >
> > > > > > --
> > > > > > Jean-Baptiste Onofré
> > > > > > [hidden email]
> > > > > > http://blog.nanthrax.net
> > > > > > Talend - http://www.talend.com
> > > > > >
> > > >
> > >
> >
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

jgoodyear
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 <[hidden email]> 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
> <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> > > 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. <[hidden email]>
> > > > 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
> > > > > <[hidden email]> 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é <
> > > [hidden email]
> > > > >
> > > > > > 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é <
> > > > > [hidden email]>
> > > > > > > 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é
> > > > > > > >> [hidden email]
> > > > > > > >> http://blog.nanthrax.net
> > > > > > > >> Talend - http://www.talend.com
> > > > > > >
> > > > > > > --
> > > > > > > Jean-Baptiste Onofré
> > > > > > > [hidden email]
> > > > > > > http://blog.nanthrax.net
> > > > > > > Talend - http://www.talend.com
> > > > > > >
> > > > >
> > > >
> > >
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

clebertsuconic
I'm getting too old now to realize that the most expensive thing in
*any* messaging system is testing.
I know you could say this to any system, but in messaging in general
this is prominent.

So, I'm not too concerned with the change itself as I am with the
amount of testing done. I have seen many cases where trusting the
testsuite alone was not enough.


So, as long as you make enough of tests (running the whole testsuite
and guaranteeing no regressions is the minimal) and run lots of
prodction like tests... it should be fine.


- Agua mole, pedra dura, tanto bate ate que fura
* Random Brazilian saying, just because it seems nice to answer with
poetry on this thread.

On Tue, Nov 27, 2018 at 10:34 AM Jamie G. <[hidden email]> 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 <[hidden email]> 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
> > <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> > > > 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. <[hidden email]>
> > > > > 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
> > > > > > <[hidden email]> 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é <
> > > > [hidden email]
> > > > > >
> > > > > > > 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é <
> > > > > > [hidden email]>
> > > > > > > > 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é
> > > > > > > > >> [hidden email]
> > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > >> Talend - http://www.talend.com
> > > > > > > >
> > > > > > > > --
> > > > > > > > Jean-Baptiste Onofré
> > > > > > > > [hidden email]
> > > > > > > > http://blog.nanthrax.net
> > > > > > > > Talend - http://www.talend.com
> > > > > > > >
> > > > > >
> > > > >
> > > >



--
Clebert Suconic
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

gtully
In reply to this post by jgoodyear
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. <[hidden email]> 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 <[hidden email]> 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
> > <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> > > > 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. <[hidden email]>
> > > > > 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
> > > > > > <[hidden email]> 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é <
> > > > [hidden email]
> > > > > >
> > > > > > > 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é <
> > > > > > [hidden email]>
> > > > > > > > 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é
> > > > > > > > >> [hidden email]
> > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > >> Talend - http://www.talend.com
> > > > > > > >
> > > > > > > > --
> > > > > > > > Jean-Baptiste Onofré
> > > > > > > > [hidden email]
> > > > > > > > http://blog.nanthrax.net
> > > > > > > > Talend - http://www.talend.com
> > > > > > > >
> > > > > >
> > > > >
> > > >
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

artnaseef
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 <[hidden email]> 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. <[hidden email]> 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 <[hidden email]> 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
> > > <[hidden email]> 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 <[hidden email]>
> 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 <
> [hidden email]>
> > > > > 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. <
> [hidden email]>
> > > > > > 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
> > > > > > > <[hidden email]> 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é <
> > > > > [hidden email]
> > > > > > >
> > > > > > > > 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é <
> > > > > > > [hidden email]>
> > > > > > > > > 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é
> > > > > > > > > >> [hidden email]
> > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > [hidden email]
> > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > Talend - http://www.talend.com
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
>
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

jgenender
In reply to this post by gtully
gtully 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.

I am a bit confused and surprised.  Its been maintained for 7 years by a
small select few of what has become stovepipe app development.
MessageDatabase alone has over 4000 lines of convoluted and very hard to
follow Java code.  Jamie appears to have done a heck of a service to the
community by simply cleaning some of KahaDB up and making things more
readable. You asked "Easy to maintain for who"?  The entire community.  Did
you look at his changes?  You may find them to be a gain for you.  Your
comment above evinces ownership of a codebase which is contrary to making
things right for the community.  This disenfranchises further and future
contributions to the codebase.

As it stands, you are certainly the expert when it comes to KahaDB.  This is
a good opportunity to make this more maintainable so that you are not the
only go-to person when it comes to this code area.  If I was in your shoes,
I would welcome this change because it relieves you of having to be a single
point for all these changes to go through.  When I see comments of "I am too
busy, I can get to it next week" or other committers saying "Gary is the
expert, I want his eyes on it before committing it", this becomes very
concerning.


gtully wrote
> 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.

This *is* the beginning of a release cycle.  5.16.0 is a perfect place and
time to do this.  5.15.X train is about to end and it makes much more sense
from a maintainability perspective to do this now.  That is to your point
about having to backport.  Doing it now removes that as a pain point.  I
really think that a reasonable review of what Jamie has done from you could
be beneficial as you may be delightfully surprised at what was done.

I believe Jamie and Arthur have put a lot of thought, time, and sweat into
starting this effort, and it was done as a benefit to the community, not
personally.  I believe you have a wealth of experience in this area and it
would be great to have you support the effort so it makes it even easier on
you for multiple reasons.

Waiting for 5.17.0 for something that really is non-invasive is asking quite
a lot.  If 5.17.0 is to be released in the next few weeks, then sure, lets
do that.  But we both know 5.17.0 (if it follows history) likely won't be
released until 2020.

This is not a major refactor.  Its just code clean up and making the code
more modular.  Your input would really be appreciated.

Jeff



--
Sent from: http://activemq.2283324.n4.nabble.com/ActiveMQ-Dev-f2368404.html
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

jgoodyear
In reply to this post by clebertsuconic
Hi Clebert,

I've run the full set of test suites with these changes to passing tests.

I'm happy to expand upon unit testing of course.
On Tue, Nov 27, 2018 at 3:53 PM Clebert Suconic
<[hidden email]> wrote:

>
> I'm getting too old now to realize that the most expensive thing in
> *any* messaging system is testing.
> I know you could say this to any system, but in messaging in general
> this is prominent.
>
> So, I'm not too concerned with the change itself as I am with the
> amount of testing done. I have seen many cases where trusting the
> testsuite alone was not enough.
>
>
> So, as long as you make enough of tests (running the whole testsuite
> and guaranteeing no regressions is the minimal) and run lots of
> prodction like tests... it should be fine.
>
>
> - Agua mole, pedra dura, tanto bate ate que fura
> * Random Brazilian saying, just because it seems nice to answer with
> poetry on this thread.
>
> On Tue, Nov 27, 2018 at 10:34 AM Jamie G. <[hidden email]> 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 <[hidden email]> 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
> > > <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> > > > > 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. <[hidden email]>
> > > > > > 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
> > > > > > > <[hidden email]> 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é <
> > > > > [hidden email]
> > > > > > >
> > > > > > > > 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é <
> > > > > > > [hidden email]>
> > > > > > > > > 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é
> > > > > > > > > >> [hidden email]
> > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > [hidden email]
> > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > Talend - http://www.talend.com
> > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
>
>
>
> --
> Clebert Suconic
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

gtully
In reply to this post by artnaseef
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 <[hidden email]> 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 <[hidden email]> 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. <[hidden email]> 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 <[hidden email]> 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
> > > > <[hidden email]> 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 <[hidden email]>
> > 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 <
> > [hidden email]>
> > > > > > 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. <
> > [hidden email]>
> > > > > > > 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
> > > > > > > > <[hidden email]> 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é <
> > > > > > [hidden email]
> > > > > > > >
> > > > > > > > > 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é <
> > > > > > > > [hidden email]>
> > > > > > > > > > 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é
> > > > > > > > > > >> [hidden email]
> > > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > > [hidden email]
> > > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > > Talend - http://www.talend.com
> > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> >
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

jgoodyear
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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. <[hidden email]> 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 <[hidden email]> 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
> > > > > <[hidden email]> 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 <[hidden email]>
> > > 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 <
> > > [hidden email]>
> > > > > > > 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. <
> > > [hidden email]>
> > > > > > > > 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
> > > > > > > > > <[hidden email]> 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é <
> > > > > > > [hidden email]
> > > > > > > > >
> > > > > > > > > > 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é <
> > > > > > > > > [hidden email]>
> > > > > > > > > > > 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é
> > > > > > > > > > > >> [hidden email]
> > > > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > > > >
> > > > > > > > > > > --
> > > > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > > > [hidden email]
> > > > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > > > Talend - http://www.talend.com
> > > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > >
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

clebertsuconic
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. <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> 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. <[hidden email]>
> 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 <[hidden email]>
> 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
> > > > > > <[hidden email]> 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 <[hidden email]>
> > > > 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 <
> > > > [hidden email]>
> > > > > > > > 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. <
> > > > [hidden email]>
> > > > > > > > > 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
> > > > > > > > > > <[hidden email]> 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é <
> > > > > > > > [hidden email]
> > > > > > > > > >
> > > > > > > > > > > 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é <
> > > > > > > > > > [hidden email]>
> > > > > > > > > > > > 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é
> > > > > > > > > > > > >> [hidden email]
> > > > > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > > > > >
> > > > > > > > > > > > --
> > > > > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > > > > [hidden email]
> > > > > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > > > > Talend - http://www.talend.com
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > >
>
--
Clebert Suconic
Reply | Threaded
Open this post in threaded view
|

Re: [Discuss] Refactoring KahaDBStore class

christopher.l.shannon
I'm actually working on getting JDK 11 support right now.  The idea is
ActiveMQ will still have a compile/runtime target of JDK 8 but will run
without issue on JDK 11 and be able to be built with JDK 11 etc.  We need
to have that done before going to 5.16.0 as long as a bunch of dependency
updates.  Otherwise there isn't anything else stopping us from doing a
5.16.0

On Thu, Nov 29, 2018 at 9:03 AM Clebert Suconic <[hidden email]>
wrote:

> 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. <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> > 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. <[hidden email]>
> > 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 <[hidden email]
> >
> > 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
> > > > > > > <[hidden email]> 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 <
> [hidden email]>
> > > > > 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 <
> > > > > [hidden email]>
> > > > > > > > > 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. <
> > > > > [hidden email]>
> > > > > > > > > > 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
> > > > > > > > > > > <[hidden email]> 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é <
> > > > > > > > > [hidden email]
> > > > > > > > > > >
> > > > > > > > > > > > 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é <
> > > > > > > > > > > [hidden email]>
> > > > > > > > > > > > > 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é
> > > > > > > > > > > > > >> [hidden email]
> > > > > > > > > > > > > >> http://blog.nanthrax.net
> > > > > > > > > > > > > >> Talend - http://www.talend.com
> > > > > > > > > > > > >
> > > > > > > > > > > > > --
> > > > > > > > > > > > > Jean-Baptiste Onofré
> > > > > > > > > > > > > [hidden email]
> > > > > > > > > > > > > http://blog.nanthrax.net
> > > > > > > > > > > > > Talend - http://www.talend.com
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > >
> >
> --
> Clebert Suconic
>
12