[dev] Horde_Share_Base::listShares()

[Gunnar Wrobel]

Zitat von Jan Schneider <jan at horde.org>:

> Zitat von Michael Rubinsky <mrubinsk at horde.org>:
>
>> Quoting Jan Schneider <jan at horde.org>:
>>
>>> Zitat von Gunnar Wrobel <wrobel at horde.org>:
>>>
>>>> Hi!
>>>>
>>>> The function mentioned above does support several sorting/paging  
>>>> features ('from', 'count', 'sort_by', 'direction'). As far as I  
>>>> can tell none of these parameters are actively used within our  
>>>> code base (with the exception of 'sort_by' but 'name' seems to be  
>>>> the only setting used here). Currently only the Sql-Drivers  
>>>> supports these features and I wonder if we really need them.
>>
>> Huh. That's embarrasing. I'm pretty sure it worked at one point,  
>> but maybe that was back in the DT days. I never realised that the  
>> paging parameters were ignored by the SQL share drivers.

They don't. As mentioned in my original mail the SQL drivers are the  
only ones that actually support it. It does not work within the  
Horde_Share_Base::listShares() but that only applies to the Datatree  
and Kolab drivers.

>> I know that Ansel passes these values when performing certain  
>> searches, like listing all of a user's galleries, or most recent  
>> galleries, or any number of other searches. The View code in Ansel,  
>> however, also honors the per-page prefs, which is probably why I  
>> never noticed this.

Ah, I overlooked that it is indeed used there.

>>
>> Ansel could potentially generate a large number of shares, since  
>> each gallery/subgallery is a separate share - though I imagine it  
>> would be rare to reach the thousands per user number. I can  
>> understand the issue of large session caches in certain cases.
>>
>> I have mixed feelings about not implementing this though. On the  
>> one hand, if the user *does* have a large number of shares - and  
>> I'm honestly not sure how large that number would have to be to  
>> start beccoming an issue - then it seems a waste to carry around  
>> potentially the user's entire share list in the session if all that  
>> was ever needed were the first 9 shares. For example - imagine I  
>> have 100 Ansel galleries. I log into Horde, and since Ansel is my  
>> initial application, I load the first page of my gallery list, I  
>> then move on to reading email and don't go back to Ansel again  
>> during this session.  Without support for paging, this would cause  
>> all 100 of my ansel shares to be carried around for the life of my  
>> session...and from what you are saying this is currently happening.

No, it is indeed working as you expect it to. I didn't see it this way  
as I assumed the listShare() parameters could vary a lot and you might  
end up with many overlapping and redundant segments of the same list.  
But you are probably right and in most cases the list parameters will  
be quite regular.

>>
>> So I guess the question is: Is it more expensive to keep more  
>> search result entries in the list cache - each with a smaller  
>> subset of shares, or to keep a smaller number of search results,  
>> but each with larger numbers of shares?
>>
>> I think the case where a user would load thousands of shares, then  
>> sort them in multiple ways is a fringe case. More likely, a smaller  
>> subset of those shares would be loaded at a time e.g., when paging  
>> through a gallery list.

I agree.

>>
>> Another option is to ignore the paging/sorting parameters when  
>> caching and hitting the DB, and apply to paging limits in PHP, this  
>> has the disadvantage of having to apply the sort to the share list  
>> each time, prior to applying the limits. Since these parameters are  
>> not currently being passed to the DB anyway, this is probably what  
>> we are already doing in the application code.

For Kolab I will have to do the paging and sorting in PHP but of  
course it makes sense to delegate the task to the backend in case it  
supports it.

>>
>>>> Horde_Share_Base::listShares() is fully replaced within  
>>>> Horde_Share_Sql and Horde_Share_Sql_Hierarchical.
>>
>> Doing it this way is probably historical cruft, left over from when  
>> these drivers were written, and DT was the only supported driver.
>
> No, this is exactly for the paging, because overwriting the default  
> method in the base class (which should do the paging/sorting in PHP  
> code) allows to implement optimized paging/sorting in the backend.
> It's well possible that this doesn't work anymore, but I'm pretty  
> sure this was the original idea behind it.

Yes, I agree. I just need to repair the paging/sorting in  
Horde_Share_Base::listShares() then. For drivers not overwriting  
listShares but instead using _listShares the whole list of shares gets  
stored in the _listcache though. That is something I will keep that  
way as the backend returns the full list anyway. I could imagine that  
one could also store the sorted/paged results in the cache to avoid  
the cycles that sorting will take. But at the moment only ansel uses  
this feature and there is only the SQL driver for ansel so that  
wouldn't make much sense.

>
>>>> For Horde_Share_Kolab and Horde_Share_Datatree  
>>>> Horde_Share_Base::listShares() is called which delegates to  
>>>> Horde_Share_{Datatree,Kolab}::_listShares(). The sorting/paging  
>>>> apparently should be handled within  
>>>> Horde_Share_Base::listShares() for these drivers. The current  
>>>> implementation is incomplete however.
>>
>> Now that DT is deprecated, if we decide to implement the paging in  
>> the DB, we could probably just make Horde_Share_Base::listShares()  
>> abstract. Otherwise, yeah, we could just implement it in  
>> Horde_Share_Base::listShares()

I don't mind either way. The current structure has the side effect of  
forcing you to add the _listShares() method to the SQL driver which is  
kind of useless. But if some other driver than SQL based and Kolab  
would come along it might benefit from the current structure.

>>
>>>> I don't mind completing the implementation if necessary but there  
>>>> are some things that currently puzzle me. Assuming a user has a  
>>>> few thousand shares and we would use the paging feature for  
>>>> example: Wouldn't the current Horde_Share_Sql:listShares()  
>>>> quickly fill the Horde_Share_Base::_listcache with plenty of  
>>>> caching information that gets stored in the session? For every  
>>>> sort setting it would be the same thing.
>>
>> This is assuming that all thousand shares would be returned in each  
>> query. If we implement paging, it's likely only a small subset of  
>> those shares would be returned for any one query - the amount that  
>> would fit on a page. The exception to this is when querying for  
>> things like select lists, but those would likely not be requested  
>> with more than a single sort parameter.  Generally, though, yes,  
>> each sort/page combmination would be cached separately.
>>

As mentioned above I see now that the current scheme makes sense.

>>>> I also wonder a bit if that is actually information that should  
>>>> go into the session at all.
>>
>> I know that in FW_3, at least at one time, it greatly improved  
>> performance since share queries tend to be expensive, especially on  
>> mysql. Jan could probably answer this part better than I, since he  
>> has done some benchmarking recently.
>
> Yes, it improves performance a lot. And IIRC Michael has already  
> reduced session storage a lot by implementing Serializable.
> It might still offer enough benefit if this cached in a Horde_Cache  
> backend instead of the session though. This would be separate from  
> existing share caching though.

Caching was no question. I was just wondering if it should be part of  
the session. But if the list really does not get longer than the share  
list then I'm not really worried (anymore).

>
>>>> On the other hand there has been a lot of work put into the Share  
>>>> driver recently and as far as I understand it there is more to  
>>>> come. I just wanted to get some feedback to understand where this  
>>>> is heading and if I should ignore the topic for now.
>>>
>>> If we don't use some of the paging/sorting features and don't come  
>>> up with any use case for it (and honestly, who is going to have  
>>> such a huge load of shares?), I'm fine with removing them to  
>>> reduce complexity.
>>
>> Honestly not 100% sure which way I'd prefer, but I think I'm  
>> leaning towards having the DB honor the parameters, instead of  
>> having to sort/page only in pure php.


I would then start fixing the current situation soon. After all the  
Kolab backend is the only that will currently use the PHP based  
sorting/paging.

Two additional questions though:

1) Why do we have a separate Sql and Sql_Hierarchical driver? Wouldn't  
it make sense to collapse the two and always be hierarchical? I'd have  
no problems supporting a tree structure with the Kolab driver. In fact  
I'd like to do that.

2) Can we move the handling of credentials into the backend? I admit I  
didn't go through the full Share code yet so I might not know all  
implications of this. But my impression so far was that the Sql driver  
operates in "Admin" mode. It has full access to the DB and can do  
things like listAllShares() - independant of the permissions. On the  
other hand listShares() needs a specific user ID. I'm unable to  
support this scheme with the Kolab backend. Simply because ACLs are  
*always* applied in the backend. The IMAP connection has been  
authenticated for a specific user and I'm not able to escape the  
boundaries of this user. I could of course list all shares on the  
server but I would have to authenticate as "manager" (or whatever the  
root/admin IMAP account is named) to the server. I don't mind the  
listAllShares() method so much but I would like to get rid of the  
$userid parameter in listShares().

Cheers,

Gunnar

>>
>>
>> Thanks,
>> mike
>>
>> --
>> The Horde Project (www.horde.org)
>> mrubinsk at horde.org
>>
>> "Time just hates me. That's why it made me an adult." - Josh Joplin
>> -- 
>> Horde developers mailing list
>> Frequently Asked Questions: http://horde.org/faq/
>> To unsubscribe, mail: dev-unsubscribe at lists.horde.org
>>
>
>
>
> Jan.
>
> -- 
> Do you need professional PHP or Horde consulting?
> http://horde.org/consulting/
>
> -- 
> Horde developers mailing list
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe at lists.horde.org

-- 
Core Developer
The Horde Project

e: wrobel at horde.org
t: +49 700 6245 0000
w: http://www.horde.org

pgp: 9703 43BE
tweets: http://twitter.com/pardus_de
blog: http://log.pardus.de