On Wed, Aug 10, 2016 at 12:41 PM, Fabian Pichler <[email protected]> wrote: > ok, if I understand correctly the API contract so far is, that the context > defines the database used and the reference of the document to load > must only be taken relative to the given context. (old String fullName > and context-database interpretation).
Yes. > > So, I think it would be better to compute the lookup key based on the > context database and the fullName computed out of the document reference. > Otherwise you would not get the same document in both situations. > 1. from cache > 2. freshly loaded Yes this what I explained. > > Anyway, I am not sure if it is a good idea to fix the key-issue as part of > XWIKI-13623. I think that XWIKI-13623 should only focus on the mentioned > memory visability and resulting data loss issue. I did not told you to do it as part of XWIKI-13623 but you cannot do XWIKI-13623 without cleaning a bit the way cache key is handled right now in the method since from what I understood you depend on a proper cache key (to access the right DocumentLoader) in the solution you proposed. > > Fabian. > > 2016-08-10 10:52 GMT+02:00 Thomas Mortagne <[email protected]>: > >> On Wed, Aug 10, 2016 at 10:14 AM, Fabian Pichler >> <[email protected]> wrote: >> > Hi Thomas >> > >> > Thanks for responding and thanks for supporting the suggested solution. >> > >> > I understand the doc.Key-issue that you mentioned. Yet, I think this is >> not >> > a >> > XWikiCacheStore#loadXWikDoc concern. In my understanding the expected >> > document >> > to load is defined by the document-reference and the language. Thus the >> > Hibernate-Store must be concerned in setting the context database >> according >> > to the Wiki-Reference before loading the document. This is acctually >> already >> > done in the XWiki#getDocument call. I agree that this might be the wrong >> > place to do it. >> > Thus, I would suggest to fix this issue in the XWikiHibernateStore on a >> > seperate jira-issue. >> >> In an ideal world it would be fixed in XWikiHibernateStore I agree but >> this would break retro compatibility (yeah always a pain). That's why >> I talked about providing a new API. >> >> So right now in practice using a combination of context wiki and >> document reference is part of the API definition and can be applied at >> XWikiCacheStore too. >> >> Note that in practice this is actually a pretty rare case since anyone >> is supposed to use XWiki#getDocument which make sure the context >> contain the document wiki. But this can still happen since this is a >> very old and public API. >> >> > Maybe this could be combined with a significant performance improvment in >> > loading >> > the documents with fewer sql-queries per loading action. >> > >> > With XWIKI-13623 I try to tackle the issue about randomly loosing >> BaseObject >> > data because of an not thread safe construction and publication of >> > XWikiDocument >> > java objects in XWikiCacheStore. Because this issue gave us a big >> headache >> > in >> > production multiple times, I solved it in our project in the way >> mentioned. >> > >> > If the xwiki-dev team supports the suggested solution, I would be willing >> > to proceed >> > and prepare a pull request for an adapted XWikiCacheStore fix. >> > >> > Kind regards, >> > Fabian >> > >> > >> > 2016-08-08 12:07 GMT+02:00 Thomas Mortagne <[email protected]>: >> > >> >> XWikiCacheStore#loadXWikiDoc is actually a bit tricker than that right >> >> now for retro compatibility and laziness (really need to rewrite this >> >> whole store API) reasons and before applying what you propose the way >> >> it deals with document uid should be modified a bit. >> >> >> >> Hibernate store is always loading the document from the wiki indicated >> >> in the context completely ignoring the wiki in the XWikiDocument >> >> object. That means that doc.getKey() may be different than the key of >> >> the document that will actually come from the hibernate store. So >> >> before adding DocumentLoader jingling a first thing to do is generate >> >> the initial cache key with a mix of the wiki id located in the context >> >> and the XWikiDocument reference. >> >> >> >> Then what you propose sounds good. >> >> >> >> On Mon, Aug 8, 2016 at 8:53 AM, Fabian Pichler >> >> <[email protected]> wrote: >> >> > Hi devs >> >> > >> >> > http://jira.xwiki.org/browse/XWIKI-13623 >> >> > I would like to discuss the suggested solution, before I start >> >> implementing >> >> > it: >> >> > >> >> > *I suggest the following solution:* >> >> > Introducing a nested DocumentLoader class which loads the document >> >> calling >> >> > the basestore loadXWikiDoc in a synchronized getDocOrLoad method. The >> >> > DocumentLoader objects gets instantiated in the cacheStore on a cache >> >> miss. >> >> > It then gets stored in a ConcurrentHashMap using the same key which is >> >> used >> >> > for the document cache (DocumentReference including language). Using >> >> > putIfAbsent to insert the DocumentLoader and calling the synchronized >> >> load >> >> > method on the object in the map solves the memory visibility issue and >> >> > moreover ensures that the same XWikiDocument is not loaded multiple >> times >> >> > concurrently from the database. It is still possible to load different >> >> > documents in parallel. A DocumentLoader will load the XWikiDocument >> only >> >> > once and the returns the identical document object on any following >> call >> >> of >> >> > the synchronized getDocOrLoad method. >> >> > >> >> > WDYT? >> >> > >> >> > Kind Regards, >> >> > Fabian >> >> > _______________________________________________ >> >> > devs mailing list >> >> > [email protected] >> >> > http://lists.xwiki.org/mailman/listinfo/devs >> >> >> >> >> >> >> >> -- >> >> Thomas Mortagne >> >> _______________________________________________ >> >> devs mailing list >> >> [email protected] >> >> http://lists.xwiki.org/mailman/listinfo/devs >> >> >> > _______________________________________________ >> > devs mailing list >> > [email protected] >> > http://lists.xwiki.org/mailman/listinfo/devs >> >> >> >> -- >> Thomas Mortagne >> _______________________________________________ >> devs mailing list >> [email protected] >> http://lists.xwiki.org/mailman/listinfo/devs >> > _______________________________________________ > devs mailing list > [email protected] > http://lists.xwiki.org/mailman/listinfo/devs -- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
