Cool -- A) To what degree have you customized the ImageRewriter? Have you made it a pipeline unto itself (ie. with sub-rewriters)? Doing so in a standard way is a motivating factor for these proposals. B) Are you modifying the caching behavior of the existing ImageRewriter pipeline -- namely, that it's executed pre-caching? C) To the extent you can say, where are your most notable gains coming from? Optimization? Resizing? Caching? Downsampling (not impl'd in BasicImageRewriter, but prototyped here)?
FYI after some thinking about this, I'm putting together a CL that provides a transition from RequestRewriter to ResponseRewriter, and transitions ImageRewriter --> ResponseRewriter. ResponseRewriter's API, for the moment: void rewrite(HttpRequest, HttpResponseBuilder); In my CL I'm @Inject'ing a ResponseRewriterRegistry with some annotation (@Precache or similarly named) where the current ImageRewriter is injected, for equivalent behavior. Note I'm starting w/ a return value of void to essentially punt on the caching infrastructure, to allow this to be done later -- instead using the status quo "where you inject is what caching you get" behavior. Obviously all this will be soon subject to codereview scrutiny :) --j On Tue, May 4, 2010 at 7:11 PM, Chirag Shah <[email protected]> wrote: > FWIW at Yahoo! we've configured a version of apache traffic server [1] > to act as a reverse proxy which hits our own ImageRewriter. We've seen > very notable performance gains with this setup. > > [1] http://trafficserver.apache.org/docs/v2/admin/reverse.htm > > On Tue, May 4, 2010 at 1:00 PM, John Hjelmstad <[email protected]> wrote: > > We've implemented several RequestRewriters that aren't in Shindig, but > > they'd be trivial to refactor into ResponseRewriters as described here. > > > > I'm unaware of anyone who's implemented a custom ImageRewriter (beyond > > BasicImageRewriter). > > > > What more-radical reorganization might you suggest? Any additional ideas > > beyond those suggested? > > > > At one point it seemed like a good idea to consolidate GadgetRewriter and > > Request||ResponseRewriter somehow, but the differences still seem notable > > enough as to disqualify that idea. > > > > --j > > > > On Tue, May 4, 2010 at 12:56 PM, Paul Lindner <[email protected]> wrote: > > > >> How many people have implemented custom rewriters? I'm guessing it's a > >> small number, in which case I'd suggest a more radical reorganization of > >> the > >> code. > >> > >> In any case these are all good ideas. > >> > >> On Fri, Apr 30, 2010 at 5:13 PM, John Hjelmstad <[email protected]> > wrote: > >> > >> > Hi all, > >> > > >> > I'm looking into the rewriting pipeline, specifically for HTTP > responses, > >> > with a few goals in mind: > >> > 1. Consolidate ImageRewriter with RequestRewriter, so it's not just a > >> > one-off rewriter injection. > >> > - And to simplify the rewriter interfaces overall, to allow for > uniform > >> > rewriter profiling code etc. > >> > 2. Expand RequestRewriters' capability to modify HTTP response > headers. > >> > 3. Maintain - and expand - our ability to cache the result of > rewriting > >> > passes intelligently. Especially important for ImageRewriter, the > output > >> of > >> > which is presently cached for notable performance gains. > >> > > >> > To this end, I've created the following CL as a starting point. It > builds > >> > upon the CL I recently submitted adding the ability to manipulate > >> > MutableContent by byte-array: > http://codereview.appspot.com/1032042/show > >> > > >> > Downstream of this CL, I'm proposing to change the rewriter interface > >> > roughly as follows: > >> > > >> > OLD > >> > // Return true if content modified. > >> > boolean RequestRewriter.rewrite(HttpRequest, HttpResponse, > >> MutableContent); > >> > > >> > NEW > >> > // Return value represents caching behavior. > >> > // Naive strategy: return boolean. True = HttpRequest is a sufficient > key > >> > to > >> > cache the rewrite. Works for simple cases, but compound rewriter cases > >> may > >> > be more difficult. > >> > // More sophisticated implementation: return a RewriteCacheKey object, > >> > containing a normalized set of all inputs to the rewrite behavior > >> > (essentially a compound key). > >> > // In turn, these objects would be used by a RewriterCache manager > >> > responsible for managing these states. > >> > ??? ResponseRewriter.rewrite(HttpRequest, HttpResponseBuilder); > >> > > >> > Note that I suggest a new interface: ResponseRewriter (a more accurate > >> name > >> > for RequestRewriter anyway), so we can support both Request and > >> > ResponseRewriter implementations during transition. > >> > > >> > Alternate approaches: > >> > I also considered simply splitting out the HTTP header-manipulation > >> methods > >> > from HttpResponseBuilder into an interface: HttpResponseHeaders. > >> > RequestRewriter's API would be modified simply as: > >> > boolean RequestRewriter.rewrite(HttpRequest, HttpResponseHeaders, > >> > MutableContent); > >> > > >> > While simpler, this had the downsides: > >> > * Still involves a signature change. > >> > * Doesn't account for better caching behavior. > >> > * Involves 2 objects rather than 1 to manipulate for output. > >> > > >> > > >> > At present, I'd like to go the conservative route: > >> > 1. HttpResponseBuilder as MutableContent (@see CL) > >> > 2. Introduce ResponseRewriter interface, returning RewriteCacheKey > with > >> > simple flags: HttpRequest-is-sufficient and rewrite-is-idempotent. > >> > 3. Transition ImageRewriter to ResponseRewriter. > >> > 4. Transition RequestRewriters to ResponseRewriter interface. > >> > > >> > > >> > Thoughts? Input welcome. > >> > > >> > Cheers, > >> > John > >> > > >> > > >
