 |
 |
Hi Jacek, I'll have to look at it in more detail. Still, didn't we agree that we needed to separate QueryContext into at least two containers: one, a scratchpad for plugins to share information; two, incoming user query context; and three, derived query characteristics needed for query dispatch (but this might better be folded into the scratchpad). Anyway, QueryContext was supposed to be behavior-less: its contents should be open for use by any plugin. Hence, the main thing you get by putting getters and setters in front of its fields is complexity, convenient places for gdb breakpoints, and extra code to deal with when maintaining and accessing. I think breaking its contents into smaller pieces is a good idea, though. Name lookup needs to be broken into its own class so it doesn't clutter up the QueryContext. The plumbing can get pretty complicated if you're strict about getting data exactly from where its produced only to the points that actually need it. Query analysis keeps getting more complex, and I just got sick of re-doing the plumbing every time a new feature needed some statistic or characteristic that was already derived but sent directly to where it used to be needed. The plugin interface seems to simplify things--you don't have to worry about the plumbing as much (but you still need to care a little, as you discovered). I think splitting QueryContext as we discussed a couple months ago will be enough to clarify things. If you can find portions that are really only used by one plugin, we can move it to that plugin--each user query has its own set of plugin instances, whose existences are completely hidden from each other. -Daniel On 06/02/2014 01:32 PM, Jacek Becla wrote: > Daniel at al, > > You keep asking about feedback re existing code and how to make > it better. While looking around, I realized I am uneasy about how > open some classes are, QueryContext perhaps is the leader in that > respect. There is a bunch of very loosely coupled things in there > that are all public, pointers to QueryContext are lying around > "everywhere", and we keep poking at it from so many places > I completely lost track who is doing what when and why, who is > reading only, and who is actually modifying what. So, I tried > to mess around with it a little, please note that this is > experimental, more like to explore things. Have a look at my > branch u/jbecla/tightningQueryContext. > > I isolated _restrictors, pushed it to a dedicated class, made it > private and added modifiers/accessors. I started doing it for > others. While doing it I notices some code belongs near > QueryContext instead of QuerySession. What I have now gives me > a much better picture who is touching _resolvers. I want to try > doing something similar to other pieces wherever it makes sense > (ScanTables, Restrictors, maybe Aliases, maybe QueryMapping, etc). > > I did not want to be too intrusive so I currently did the isolation > through (multiple) inheritance, but I'd be completely open to other > options. > > I am thinking that maybe we should go one step further and take apart > QueryContext, get rid of it, and replace it with a few specialized > QueryContext-like objects, owned by QuerySession. The reason why: > I discovered that_restrictors are really touched only by > QservRestrictorPlugin...., so perhaps there is a pattern here > and certain plugins touch certain parts only. > > Please have a look and let me know what your reaction is > (the sooner the better, if I find some time here and there > I will be playing with that code more). > > Along these lines, I think I'd be in favor of tightening other > classes and introducing modifiers/accessors instead of keeping > class members public (except really trivial classes like > css/StripingParams). > > Brickbats welcome, but I am hiding far far away, so they won't > reach me :) > > Thanks, > Jacek > > ######################################################################## > Use REPLY-ALL to reply to list > > To unsubscribe from the QSERV-L list, click the following link: > https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=QSERV-L&A=1 ######################################################################## Use REPLY-ALL to reply to list To unsubscribe from the QSERV-L list, click the following link: https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=QSERV-L&A=1