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