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