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 <a...@amlinv.com> wrote: > > Hey Gary - I agree that these changes belong on a minor version increase. > > What I don't understand is the assertion that the changes between 5.15.x > and 5.16.0 need to be small. Given that the minor version bump can mean > significant changes, as long as they are backward compatible, I see no > reason to adhere to a small set of changes between 5.15.x and 5.16.0. Add > to that fact that ActiveMQ's minor releases are sometimes really major > changes (i.e. include breaking changes), and that makes even less sense. > > Is there something more to this that perhaps I'm missing? > > Making the code more maintainable by the community, as ActiveMQ is an > Apache *community* project, is the goal. As for it being maintained for 7 > years, that's great. However, I'm sure you'll agree it's not perfect, and > community improvements are welcome. > > Art > > > On Wed, Nov 28, 2018 at 3:30 AM Gary Tully <gary.tu...@gmail.com> wrote: > > > Jamie, > > you are missing my point. it is a tradeoff plain and simple. easier to > > maintain for who? It has been carefully maintained for more than 7 > > years. > > Do refactoring at the beginning of a release cycle, not at the end. > > diffs between 5.15.x and 5.16 will be meaningless otherwise. > > push out 5.16.0, which has loads of incremental (non refactored) small > > changes. Then refactor away on master for 5.17.0 and make it better in > > any way that works for the future and for you. > > On Tue, 27 Nov 2018 at 15:34, Jamie G. <jamie.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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
