Great thanks :) On Wed, Aug 17, 2016 at 10:10 AM, Fabian Pichler <[email protected]> wrote: > 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
-- Thomas Mortagne _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
