Thank you Thomas for your explanations. You are completely right, you wrote it in the first mail. The generation of the cache key is broken. I added now XWIKI-13632 for this.
If it is ok, I start preparing a PullRequest for XWIKI-13632 and then a second one for XWIKI-13623. Kind regards, Fabian 2016-08-10 16:33 GMT+02:00 Thomas Mortagne <[email protected]>: > 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 > _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
