You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by "John Hjelmstad (JIRA)" <ji...@apache.org> on 2008/08/07 23:30:44 UTC
[jira] Created: (SHINDIG-500) Make Gadget Object's content that of
the active View
Make Gadget Object's content that of the active View
----------------------------------------------------
Key: SHINDIG-500
URL: https://issues.apache.org/jira/browse/SHINDIG-500
Project: Shindig
Issue Type: Sub-task
Components: Gadget Rendering Server (Java)
Reporter: John Hjelmstad
Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (SHINDIG-500) Make Gadget Object's content that of the active View
Posted by Kevin Brown <et...@google.com>.
On Thu, Aug 28, 2008 at 12:18 PM, John Hjelmstad <fa...@google.com> wrote:
> I see fixing the rewriter's interface as largely orthogonal to the problem
> of caching rewritten content. One way or another, the rewriter takes input
> and generates output -- the output needs to be cached, and that's what this
> approach does. The described approach is designed simply to bracket the
> amount of logic owned by each class. BasicContentRewriterRegistry registers
> and rewrites; CachingContentReweriterRegistry adds a caching layer atop
> that. Per the previous thread, CachingWebRetrievalFactory is a detail I'm
> willing to remove in favor of putting *most* caching logic in a TtlCache
> wrapper. That would simplify CachingContentRewriterRegistry in that it
> could
> subclass BasicCRR.
> Q: In an earlier thread you'd suggested adding another cache, but here you
> suggest that "delegating to a shared cache" is better. Perhaps I'm
> confused,
> but isn't a shared cache still possible here, depending on the
> implementation of CacheProvider?
>
> Perhaps you're referring to a shared cache between rewritten gadget content
> and rewritten makeRequest content -- on this note, I completely agree. I'm
> just trying to find an incremental step that maintains rewritten gadget
> content caching. As indicated in other threads, AbstractHttpCache
> (rewritten
> makeRequest cache) needs to be significantly cleaned up. At that time,
> consolidation with this rewritten cache should occur.
AbstractHttpCache shouldn't be doing any rewriting at all. Fix that and the
other problems go away.
>
>
> --John
>
> On Wed, Aug 27, 2008 at 9:54 PM, Kevin Brown (JIRA) <ji...@apache.org>
> wrote:
>
> >
> > [
> >
> https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626426#action_12626426
> ]
> >
> > Kevin Brown commented on SHINDIG-500:
> > -------------------------------------
> >
> > I think that's spending entirely too much effort to avoid fixing the
> > simpler problem, which is that the rewriter has a bad interface.
> >
> > Far fewer people are going to be affected by fixing the rewriter's
> > interface than are going to be affected by mucking around with the spec
> > factories. The new code is incredibly difficult to follow, and will
> > inevitably lead to bugs.
> >
> > Using CachingWebRetrievalFactory to cache rewritten content is an even
> > worse idea, because it means using an in-memory LRU cache for caching
> > rewritten content that would normally be delegated to a shared cache,
> > forcing users to write painful deserialization routines.
> >
> > The cache for rewritten data needs to look something like
> >
> > Cache<CacheKey, CachedData>
> >
> > Where CacheKey is similar to the cache keys used for caching HTTP
> requests
> > currently, and CachedData is pretty much just a String at this point in
> > time.
> >
> > Rewriting gadget specs is only one small part of rewriting. In many ways,
> > it's the less critical part as developers make such heavy use of
> makeRequest
> > and (soon) proxied content.
> >
> > > Make Gadget Object's content that of the active View
> > > ----------------------------------------------------
> > >
> > > Key: SHINDIG-500
> > > URL: https://issues.apache.org/jira/browse/SHINDIG-500
> > > Project: Shindig
> > > Issue Type: Sub-task
> > > Components: Gadget Rendering Server (Java)
> > > Reporter: John Hjelmstad
> > > Assignee: John Hjelmstad
> > > Attachments: spec-immutable-notdoneyet.patch
> > >
> > >
> > > Step #1 of
> >
> http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
> >
> > --
> > This message is automatically generated by JIRA.
> > -
> > You can reply to this email to add a comment to the issue online.
> >
> >
>
Re: [jira] Commented: (SHINDIG-500) Make Gadget Object's content that of the active View
Posted by John Hjelmstad <fa...@google.com>.
I see fixing the rewriter's interface as largely orthogonal to the problem
of caching rewritten content. One way or another, the rewriter takes input
and generates output -- the output needs to be cached, and that's what this
approach does. The described approach is designed simply to bracket the
amount of logic owned by each class. BasicContentRewriterRegistry registers
and rewrites; CachingContentReweriterRegistry adds a caching layer atop
that. Per the previous thread, CachingWebRetrievalFactory is a detail I'm
willing to remove in favor of putting *most* caching logic in a TtlCache
wrapper. That would simplify CachingContentRewriterRegistry in that it could
subclass BasicCRR.
Q: In an earlier thread you'd suggested adding another cache, but here you
suggest that "delegating to a shared cache" is better. Perhaps I'm confused,
but isn't a shared cache still possible here, depending on the
implementation of CacheProvider?
Perhaps you're referring to a shared cache between rewritten gadget content
and rewritten makeRequest content -- on this note, I completely agree. I'm
just trying to find an incremental step that maintains rewritten gadget
content caching. As indicated in other threads, AbstractHttpCache (rewritten
makeRequest cache) needs to be significantly cleaned up. At that time,
consolidation with this rewritten cache should occur.
--John
On Wed, Aug 27, 2008 at 9:54 PM, Kevin Brown (JIRA) <ji...@apache.org> wrote:
>
> [
> https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626426#action_12626426]
>
> Kevin Brown commented on SHINDIG-500:
> -------------------------------------
>
> I think that's spending entirely too much effort to avoid fixing the
> simpler problem, which is that the rewriter has a bad interface.
>
> Far fewer people are going to be affected by fixing the rewriter's
> interface than are going to be affected by mucking around with the spec
> factories. The new code is incredibly difficult to follow, and will
> inevitably lead to bugs.
>
> Using CachingWebRetrievalFactory to cache rewritten content is an even
> worse idea, because it means using an in-memory LRU cache for caching
> rewritten content that would normally be delegated to a shared cache,
> forcing users to write painful deserialization routines.
>
> The cache for rewritten data needs to look something like
>
> Cache<CacheKey, CachedData>
>
> Where CacheKey is similar to the cache keys used for caching HTTP requests
> currently, and CachedData is pretty much just a String at this point in
> time.
>
> Rewriting gadget specs is only one small part of rewriting. In many ways,
> it's the less critical part as developers make such heavy use of makeRequest
> and (soon) proxied content.
>
> > Make Gadget Object's content that of the active View
> > ----------------------------------------------------
> >
> > Key: SHINDIG-500
> > URL: https://issues.apache.org/jira/browse/SHINDIG-500
> > Project: Shindig
> > Issue Type: Sub-task
> > Components: Gadget Rendering Server (Java)
> > Reporter: John Hjelmstad
> > Assignee: John Hjelmstad
> > Attachments: spec-immutable-notdoneyet.patch
> >
> >
> > Step #1 of
> http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>
Re: [jira] Commented: (SHINDIG-500) Make Gadget Object's content that of the active View
Posted by John Hjelmstad <fa...@google.com>.
Thanks, Ian! That did the trick.
On Fri, Aug 29, 2008 at 3:30 AM, Ian Boston (JIRA) <ji...@apache.org> wrote:
>
> [
> https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626926#action_12626926]
>
> Ian Boston commented on SHINDIG-500:
> ------------------------------------
>
>
> The Gadget content is re-written but the the view is not.
>
> add the following to GadgetRenderTask.outputHtmlGadget
>
> String content = view.getContent();
> for (GadgetContentFilter filter : filters) {
> content = filter.filter(content);
> }
>
> + System.err.println("Gadget Content is "+gadget.getContent());
> + System.err.println("Gadget View is "+view.getContent());
>
> markup.append(content)
> .append("<script>gadgets.util.runOnLoadHandlers();</script>")
> .append("</body></html>");
>
> > Make Gadget Object's content that of the active View
> > ----------------------------------------------------
> >
> > Key: SHINDIG-500
> > URL: https://issues.apache.org/jira/browse/SHINDIG-500
> > Project: Shindig
> > Issue Type: Sub-task
> > Components: Gadget Rendering Server (Java)
> > Reporter: John Hjelmstad
> > Assignee: John Hjelmstad
> > Attachments: gadgetspec-immutable.patch
> >
> >
> > Step #1 of
> http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>
[jira] Updated: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad updated SHINDIG-500:
-----------------------------------
Attachment: (was: shindig-500.diff)
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622042#action_12622042 ]
John Hjelmstad commented on SHINDIG-500:
----------------------------------------
Hmmm.. bummer, though I'm glad we agree GadgetSpecs should be immutable, as that's my key goal here. As noted, as far as I can tell doing that requires caching of rewritten content (which I agree is of paramount importance) to occur somewhere other than (Basic)GadgetSpecFactory.
Your point about splitting/removing GadgetServer makes sense, but I'd like to find as incremental a change as possible to achieve GadgetSpec cleanup (closing in on immutability) in the meantime. The first thing that come to mind is to move the LruCache as written in BasicGadgetSpecFactory into GadgetServer (yes, making it even larger... perhaps an eventual forcing function for the change you're suggesting :)) to maintain caching semantics for rewritten content.
Assuming everyone using Shindig is also using BasicGadgetSpecFactory or a subclass of it, that ought to be acceptable (if notably suboptimal). But I have no idea if that's so -- thoughts?
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: shindig-500.diff
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad updated SHINDIG-500:
-----------------------------------
Attachment: gadgetspec-immutable.patch
Attaching latest patch making gadgetSpec immutable, cleaning up its interface, adding setContent() to Gadget, adding ContentRewriterRegistry.rewriteGadget(), and pulling up rewriting logic into GadgetServer (until it's moved again to the new rendering pipeline). The CL also maintains caching of rewritten gadget specs, for good measure... once submitted, we can move to the more satisfying task of cleaning up APIs to obviate the need for additional layers of caching, as commented here.
All unit tests, including new ones, pass just fine, but for some reason I can't figure out my client errors on java/server's EndToEndTest. Reason: 404 on /gadgets/testframework.js, which should have nothing to do with my changes.
In printf-debugging (Eclipse is hugely painful for this one) this, I discovered that the rewriter is doing precisely the same thing with and without my changes: rewriting <script src="testframework.js"/> to <script src="/gadgets/concat?stuff..."/>. Why /gadgets/testframework.js 404s then, I don't know. Ian, any clues?
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: gadgetspec-immutable.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad resolved SHINDIG-500.
------------------------------------
Resolution: Fixed
Resolved by r691437.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: gadgetspec-immutable.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "Kevin Brown (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626346#action_12626346 ]
Kevin Brown commented on SHINDIG-500:
-------------------------------------
That sounds like overkill. You already have a base rewriter that can handle many types of rewriting operations in the future, so you should just put it there.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: spec-immutable-notdoneyet.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "Kevin Brown (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622043#action_12622043 ]
Kevin Brown commented on SHINDIG-500:
-------------------------------------
That's also unnecessary. If you want to make GadgetSpec immutable, just get rid of the setters on views, and cache the rewritten content in a separate cache. Why would you have to move the GadgetSpec cache anywhere? Just make a new cache...
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: shindig-500.diff
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626411#action_12626411 ]
John Hjelmstad commented on SHINDIG-500:
----------------------------------------
Well, since a "full" rewriting operation is the composition of rewrites performed by all ContentRewriters in the Registry, I think that's the logical best place for caching functionality to be added. As such, what I've done is the following:
1. Augment ContentRewriterRegistry with a new method rewriteGadget(GadgetContext, Gadget). This in turn - for now, until we clean up the ContentRewriter interface - just runs through all rewriters calling ContentRewriter.rewriteGadgetView(...).
- While this somewhat conflates "registration" with rewriter execution, I think the combination makes reasonable sense. The registry should know how to execute the rewriters, in what order, with what optimizations, etc.
- Note, this method signature is likely subject to change once CR's interface is simplified.
2. BasicContentRewriterRegistry implements this method by iterating through ContentRewriters. That's it.
3. Introduce CachingContentRewriterRegistry, which extends CachingWebRetrievalFactory. I just introduced this latter class (which already has a bad name... gotta fix that), which abstracts away all the caching behavior from BasicGadgetSpecFactory and BasicMessageBundleFactory (these two classes have been de-duplicated now, both extending the new one).
- Because of the inheritance, CachingContentRewriterRegistry actually delegates to its own contained instance of BasicContentRewriterRegistry, basically using it as a helper class.
- CCRR thus provides configured caching capability for rewritten Gadget objects.
- Note, my implementation currently inherits the same configuration values as the gadget-spec cache for backward compatibility.
Thoughts on this approach? I'll attach a patch to this CL including this implementation shortly, after finishing some tests.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: spec-immutable-notdoneyet.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "Ian Boston (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626919#action_12626919 ]
Ian Boston commented on SHINDIG-500:
------------------------------------
This is nothing to do with your changes, I think there is a problem with the setup of the EndToEnd service.
testframework.js is part of the CDATA or the test gadgets
the tags not being re-written
but it appears to not be available on the test server for some reason
I am looking into it.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: gadgetspec-immutable.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad updated SHINDIG-500:
-----------------------------------
Attachment: (was: spec-immutable-notdoneyet.patch)
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626342#action_12626342 ]
John Hjelmstad commented on SHINDIG-500:
----------------------------------------
You have a good point -- we should keep both. I'm presently thinking about introducing a "RewriterManager" - interface or concrete class less certain at the immediate moment - which performs rewrite operations on raw input and provides complementary caching services to that. This idea isn't fully baked yet, so I'm playing around a bit with the code to see what issue(s) surface with introducing such a concept.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: spec-immutable-notdoneyet.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad reassigned SHINDIG-500:
--------------------------------------
Assignee: John Hjelmstad
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: shindig-500.diff
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad updated SHINDIG-500:
-----------------------------------
Attachment: shindig-500.diff
Updated patch attached.
This patch cleans up rewriting logic considerably by removing the separated notion of RewrittenContent and simply treating the Gadget object's Content as current. Previously, odd logic paths existed such as the fact that Substitution was required for rewriting to work, as the View.substitute() method used its own rewrittenContent field to replace its own content.
I've discovered one noteworthy property of this change, however - GadgetSpecs are cached in *non-rewritten* form by BasicGadgetSpecFactory. This is endemic to moving the notion of rewritten content out of View and into Gadget.
Is it OK to accept this for the time being? Rewriting, where it applies, will occur more often during rendering, making it both more dynamic but also more costly CPU-wise. Since gadgets still need to opt in to rewriting, I'm thinking the hit will be small compared to the benefits, both short and longer-term.
This topic raises the questions of what purpose GadgetSpecFactory performs, and whether or not GadgetSpecs should be treated as immutable "seeds" for Gadget processing or as pre-processed Gadgets themselves (essentially codifying rewriting semantics ie. that rewritten content is as cacheable as the retrieved spec itself).
For its part, BasicGadgetSpecFactory adds two concepts atop "fetching a gadget spec":
A. An LruCache for retrieved GadgetSpecs.
B. Retrieval of type=html href=<foo> View content.
Arguably, both (A) and (B) should be performed higher in the gadget processing stack, in GadgetServer. Doing so allows GadgetServer to make more nuanced decisions as to what caching characteristics apply to content and at what levels, and opens up the opportunity to inject a Gadget cache (probably keyed on Gadget+"view name") rather than piggybacking on one built into the GadgetSpecFactory of choice.
Ideally, I'd like to move ahead with this change independent of GadgetSpecFactory refactoring, particularly since that's more intrusive to the way a Gadget Server is built. Thoughts?
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: shindig-500.diff
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad updated SHINDIG-500:
-----------------------------------
Attachment: spec-immutable-notdoneyet.patch
Attached an updated (accommodating the latest merges) patch making GadgetSpec immutable once more, and moving get/setContent into Gadget. This patch isn't complete yet, pending resolution of the rewritten-content caching issue: notably, that rewritten content is not cached in this change.
Per previous discussion, I see two ways to resolve this:
1. Introduce a new rewrittenContent cache.
2. Move BasicGadgetSpecFactory's cache up to GadgetServer (this in turn is being refactored now as well, and the cache should go with it when that happens).
Thinking on it some more, in either case there's a rewritten content cache introduced at the same level that performs content rewriting: notably, GadgetServer for now. I'll introduce that and supply a new patch. Past that we need to determine whether to keep the BasicGadgetSpecFactory cache or not (I'd vote not, as it's largely redundant).
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: spec-immutable-notdoneyet.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "Ian Boston (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626926#action_12626926 ]
Ian Boston commented on SHINDIG-500:
------------------------------------
The Gadget content is re-written but the the view is not.
add the following to GadgetRenderTask.outputHtmlGadget
String content = view.getContent();
for (GadgetContentFilter filter : filters) {
content = filter.filter(content);
}
+ System.err.println("Gadget Content is "+gadget.getContent());
+ System.err.println("Gadget View is "+view.getContent());
markup.append(content)
.append("<script>gadgets.util.runOnLoadHandlers();</script>")
.append("</body></html>");
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: gadgetspec-immutable.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "Kevin Brown (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626426#action_12626426 ]
Kevin Brown commented on SHINDIG-500:
-------------------------------------
I think that's spending entirely too much effort to avoid fixing the simpler problem, which is that the rewriter has a bad interface.
Far fewer people are going to be affected by fixing the rewriter's interface than are going to be affected by mucking around with the spec factories. The new code is incredibly difficult to follow, and will inevitably lead to bugs.
Using CachingWebRetrievalFactory to cache rewritten content is an even worse idea, because it means using an in-memory LRU cache for caching rewritten content that would normally be delegated to a shared cache, forcing users to write painful deserialization routines.
The cache for rewritten data needs to look something like
Cache<CacheKey, CachedData>
Where CacheKey is similar to the cache keys used for caching HTTP requests currently, and CachedData is pretty much just a String at this point in time.
Rewriting gadget specs is only one small part of rewriting. In many ways, it's the less critical part as developers make such heavy use of makeRequest and (soon) proxied content.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: spec-immutable-notdoneyet.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "Kevin Brown (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12626338#action_12626338 ]
Kevin Brown commented on SHINDIG-500:
-------------------------------------
No, it's not redundant. The gadget spec factory caches parsed gadget specifications -- rewritten content is a completely different thing, and moving the caching logic of gadget specs into the gadget server is inflexible for anyone who needs to produce gadget specs in a different way or who has a different caching strategy.
Cached content is a not the same as a cached gadget spec, as not everything that gets rewritten is a gadget spec (or even html, for that matter).
It definitely makes sense to have a separate cache for rewritten content (assuming that rewriting is expensive and that many rewrite operations are cacheable), but that cache should be used for *all* rewriting (the body of the gadget, a proxied css file, or makeRequest data). The best place to put it would probably be in the rewriter.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: spec-immutable-notdoneyet.patch
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622045#action_12622045 ]
John Hjelmstad commented on SHINDIG-500:
----------------------------------------
The proposed diff does remove all setters with the exception of setContent(...) used for type != URL && href != null View content retrieval, which seems reasonable to keep in for the time being (though we could do to clean that up too)... so, that sounds like a great idea. I'll add a new rewrittenContent LruCache. thanks!
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: shindig-500.diff
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad updated SHINDIG-500:
-----------------------------------
Attachment: shindig-500.diff
Patch implementing this approach attached.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Attachments: shindig-500.diff
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SHINDIG-500) Make Gadget Object's content that
of the active View
Posted by "Kevin Brown (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12622037#action_12622037 ]
Kevin Brown commented on SHINDIG-500:
-------------------------------------
No, it isn't acceptable.. As Louis and I have posted on other bugs, rewriting (and associated sub-processes) is one of the most expensive operations done by Shindig today (after RSA signing and content encoding detection). We need to optimize this in the long run anyway, but we can't afford to rewrite every render in the short term either.
The rewritten content must be cached as much as possible. You can cache the content separately from the spec, but it must be cached somewhere.
Re: SpecFactory
GadgetSpecs should be immutable. The mutable bits that have been added are a design mistake made over the last several months as feature creep has risen. There's no need for more nuanced behavior for basic retrieval, and if there was, it would just be another GadgetSpecFactory implementation, not more logic wedged into GadgetServer (which is already doing more work than it should be).
Really, GadgetServer needs to be scrapped entirely, and there should be separate classes for dealing with the two main gadget operations:
1. A code path that deals with rendering gadgets
2. A code path that deals with extracting metadata from gadgets
Both of these exist in skeleton form at the servlet level, where gadget rendering is clearly separated from metadata access, but under the covers they both use GadgetServer, which is just terrible to work with.
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
> Attachments: shindig-500.diff
>
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (SHINDIG-500) Make Gadget Object's content that of
the active View
Posted by "John Hjelmstad (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SHINDIG-500?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
John Hjelmstad updated SHINDIG-500:
-----------------------------------
Attachment: (was: shindig-500.diff)
> Make Gadget Object's content that of the active View
> ----------------------------------------------------
>
> Key: SHINDIG-500
> URL: https://issues.apache.org/jira/browse/SHINDIG-500
> Project: Shindig
> Issue Type: Sub-task
> Components: Gadget Rendering Server (Java)
> Reporter: John Hjelmstad
> Assignee: John Hjelmstad
>
> Step #1 of http://mail-archives.apache.org/mod_mbox/incubator-shindig-dev/200808.mbox/%3cab5e78ed0808061534r48c04de5j8909bc7e8316cc22@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.