Hi Tammy,
My apologies for my part of the communication breakdown; as you say, we've
been trying to include you, but given that you've really been out on
maternity leave the entire time, that hasn't really worked...
That said, we have tried to take chica's needs into account, and I do think
that we've organized things in a way that makes chica *better* off, and I
look forward to discussing and/or clarifying on Wednesday's design call.
Are you available for that? (I haven't gotten a chance to look at the
changes to logic yet, but will try to do so before Wednesday.)
The genesis of it was that we started discussing "logic 2.0", and realized
that it would not be possible to switch to a new lighter-weight design
without hosing current logic users, so we shifted to introducing the new
design as the "calculation", and leaving the current logic's design
unchanged.
The plan we had laid out is:
- Chica will own logic, and have full ownership (and veto power) over
any changes. This largely means the 0.5 version you already have running.
- Peeking at svn, the only changes since October have been: (1)
mavenizing logic, (2) switching it to use liquibase instead of direct sql
for its data model (hopefully you're okay with maven+liquibase,
but I guess
it's up to you), and (3) the changes that Wyclif committed.
- The intention of Wyclif's changes (which I haven't looked at yet) is
to add some *adapter* classes to logic such that logic's functionality
can *also* be accessed through the Calculation interfaces, but this
should have *zero* impact on Chica's usage of logic. I.e. the intention
is that doing logicService.eval("WEIGHT (KG)") will behave exactly as it
does now, and return a logic Result; and that an admin could also configure
things so that doing calculationService.eval("WEIGHT (KG)") ends up calling
that same underlying logic rule, and getting back a calculation result.
- If the implementation of the adapters that Wyclif did impose some
new design or maintenance constraints on logic (they shouldn't
need to), we
should fix that
So, to summarize, I completely agree that we want chica to own *current* logic
(basically 0.5 + maven + liquibase), and I'd hope that in the future you
all add more functionality, and undo any bad refactorings that we did
there, but the main point is that there should be no further changes to
logic that breaks things for you, because you'll be the gatekeeper.
AFAIK the only lines of code that Chica should need to change in the short
run are to go from Context.getLogicService() to
Context.getService(LogicService.class).
Further discussion on Wednesday.
-Darius
On Mon, Apr 2, 2012 at 11:02 AM, Tammy Dugan <[email protected]> wrote:
> I finally had a chance to look at all the new logic code. First I need to
> vent about a couple of things and then I will try to be constructive:
>
> * It took us MONTHS to get the last rewrite of logic to work correctly
> with openmrs 1.7.x and the chica code
> * We just got it working at the end of October 2011 and Openmrs started
> rewriting it again in November 2011.
> * The first round of design calls for the logic rewrite happened in
> mid-November. I had a two week old baby and did not have time to deal with
> it.
> * Looking back through dev emails, the logic discussion ended at the end
> of November and picked up in mid February
> * In mid-February was the first time I saw anything about Calculation.
> * On March 5th there was a dev list email from Darius that he and Mike
> would be here for one day and wanted to meet with me and some other people
> * I was still on part-time maternity leave but rearranged my schedule with
> one days notice to meet with them
> * When I met with them, I struggled to understand the logic design that I
> had not looked at since I had only been back at work for 2 weeks after
> maternity leave
> * In that meeting, Darius asked us if we could own logic. Basically we
> were supposed to own something that got completely redesigned that we had
> little or no input into the design that didn't make any sense to us and
> stuff that had been fixed repeatedly was broken yet again.
>
> Based on all this, you can see why I might be a little irritated. Yes, I
> know you tried to include me in the design but you knew I was pretty much
> unavailable. Yes, I could have said no when Darius said we should own logic
> but I felt backed into a corner. There was a breakdown in communication on
> several fronts. If I would have know we were expected to own logic, I would
> have said no to the rewrite to begin with.
>
> Venting done. Now to be constructive:
>
> * The name Calculation is a horrible name for that object. It doesn't
> calculate anything! Why not call it "RuleDefinition" since it defines the
> properties of the rule and then call CalculationEvaluator "Rule"? Why was
> Rule split into two objects anyway? Why not just add a
> getParameterDefinitionSet method to the rule interface? Why would you ever
> want some object that doesn't evaluate anything being called by logic? What
> is the point in that?
> * I think the strongly typed results and ParameterDefinitionSets are nice
> changes
>
> * Logic cannot hit the database for every token lookup. This is an
> unacceptable performance hit for a system like chica. The tokens need to be
> cached.
> * CalculationEvaluators and Calculations should not be singletons. Making
> them singletons is non-thread safe and requires special programming that is
> not intuitive to the naive programmer. A new instance should be created
> each time.
>
>
> For a naive programmer, I think the new design complicates things even
> more than before and makes it even less accessible for developers to
> contribute.
>
>
> In summary, if the chica group is going to own logic, we would be willing
> to own the 0.5 logic that we worked so hard to getting working. We are not
> willing to put the time and effort in to get the new calculation version of
> logic working.
>
>
> Thanks,
>
> Tammy
>
>
>
> On 3/29/2012 11:29 AM, Michael Seaton wrote:
>
> Hi Tammy,
>
> Is this an issue in the Calculation module? Or is this an issue with how
> logic is exposing itself to Calculation? Weren't you involved in the
> coding changes that took place during the Sprint? That was meant to be for
> you and the Chica team to contribute and own, not for anyone to impose on
> you.
>
> It does seem we should cache calculation registrations in memory, we are
> not doing this currently. But this shouldn't necessarily be an issue for
> Chica, if it is going to the logic_token_registration table for it's own
> purposes, should it? I don't really see how exposing Logic to Calculation
> is a risk for you, as long as you continue to access Logic natively as you
> always have and just provide a couple of adapter classes to expose it for
> other via the Calculation module.
>
> Mike
>
>
> On 03/29/2012 11:05 AM, Tammy Dugan wrote:
>
> There were two major issues that came up when reviewing the changes with
> Wyclif,
>
> 1. Logic cannot hit the database for every token lookup. This is an
> unacceptable performance hit for a system like chica. The tokens need to be
> cached.
> 2. Rules should not be singletons. Making them singletons is non-thread
> safe and requires special programming that is not intuitive to the naive
> programmer. A new instance should be created each time.
>
>
> I also want to point out that we have fixed #2 twice already in previous
> versions and now it needs to be fixed for a third time. Also, #1 was fixed
> in the previous version and now has to be fixed again. It gets frustrating
> that these same problems keeps getting reintroduced each time logic is
> refactored and each time I have to make the case about why it should be
> that way and we have to fix the problem. It is important when refactoring
> to preserve the old behavior!
>
> Because of these issues, we have no immediate plans to update chica to use
> the new version of logic with Calculations. It took us months to get things
> working before and we just don't have the programming resources to do it
> now or in the near future.
>
> Thanks,
>
> Tammy Dugan
>
>
> On 3/29/2012 10:08 AM, Friedman, Roger (CDC/CGH/DGHA) (CTR) wrote:
>
> It appears that trunk produces 0.5.3 which includes Calculation, last
> branch is 0.5.1 which is not mavenized, 0.5.2 is mavenized, can it be a
> branch even if not released?****
>
> ** **
>
> *From:* [email protected] [mailto:[email protected] <[email protected]>] *On
> Behalf Of *Wyclif Luyima
> *Sent:* Friday, March 23, 2012 3:53 PM
> *To:* [email protected]
> *Subject:* [OPENMRS-DEV] Calculation sprint wrap up****
>
> ** **
>
> Hi everyone,****
>
> ** **
>
> For the past 2 weeks, we have been having a sprint on the calculation
> module plus making logic and reporting to expose themselves as
> calculations. We managed to get all the 27 tickets specific to the module
> done by Wednesday. Some work has been done to retrofit logic and reporting
> to be exposed as calculations, i went through the changes in logic with
> Tammy and Win this afternoon, Tammy had some interesting points of
> discussion which i believe she will send on the dev list.****
>
> ** **
>
> In general, the code looks pretty good and i think we are ready for its
> 1.0 release which could be in the next 1 -2 weeks, apparently we have to
> wait for final high level reviews from Darius and Burke as contributors to
> its design to confirm if it is actually what they envisioned.****
>
> ** **
>
> The sprinter turn up was good which included the core developers, Mike and
> Mykola, thank you all for your relentless work.****
>
> ** **
>
> Have a great weekend.****
>
> ** **
>
> Wyclif****
>
> ** **
> ------------------------------
>
> Click here to
> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from
> OpenMRS Developers' mailing list
> ****
> ------------------------------
> Click here to
> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from
> OpenMRS Developers' mailing list
>
>
> --
> Tammy Dugan
> CHIRDL Technical Lead
> Children's Health Services Research
> IU School of Medicine
>
> ------------------------------
> Click here to
> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from
> OpenMRS Developers' mailing list
>
> ------------------------------
> Click here to
> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from
> OpenMRS Developers' mailing list
>
>
> --
> Tammy Dugan
> CHIRDL Technical Lead
> Children's Health Services Research
> IU School of Medicine
>
> ------------------------------
> Click here to
> unsubscribe<[email protected]?body=SIGNOFF%20openmrs-devel-l>from
> OpenMRS Developers' mailing list
>
_________________________________________
To unsubscribe from OpenMRS Developers' mailing list, send an e-mail to
[email protected] with "SIGNOFF openmrs-devel-l" in the body (not
the subject) of your e-mail.
[mailto:[email protected]?body=SIGNOFF%20openmrs-devel-l]
