Solr
  1. Solr
  2. SOLR-1644

Provide a clean way to keep flags and helper objects in ResponseBuilder

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: search
    • Labels:
      None

      Description

      Many components such as StatsComponent, FacetComponent etc keep flags and helper objects in ResponseBuilder. Having to modify the ResponseBuilder for such things is a very kludgy solution.

      Let us provide a clean way for components to keep arbitrary objects for the duration of a (distributed) search request.

      1. SOLR-1644.patch
        7 kB
        Noble Paul
      2. SOLR-1644.patch
        7 kB
        Noble Paul
      3. SOLR-1644.patch
        6 kB
        Noble Paul

        Activity

        Hide
        Hoss Man added a comment -

        There is no indication that anyone is actively working on this issue, so removing 4.0 from the fixVersion.

        Show
        Hoss Man added a comment - There is no indication that anyone is actively working on this issue, so removing 4.0 from the fixVersion.
        Hide
        Robert Muir added a comment -

        rmuir20120906-bulk-40-change

        Show
        Robert Muir added a comment - rmuir20120906-bulk-40-change
        Hide
        Hoss Man added a comment -

        bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment

        Show
        Hoss Man added a comment - bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment
        Hide
        Robert Muir added a comment -

        3.4 -> 3.5

        Show
        Robert Muir added a comment - 3.4 -> 3.5
        Hide
        Robert Muir added a comment -

        Bulk move 3.2 -> 3.3

        Show
        Robert Muir added a comment - Bulk move 3.2 -> 3.3
        Hide
        Hoss Man added a comment -

        Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email...

        http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

        Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed.

        A unique token for finding these 240 issues in the future: hossversioncleanup20100527

        Show
        Hoss Man added a comment - Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed. A unique token for finding these 240 issues in the future: hossversioncleanup20100527
        Hide
        Noble Paul added a comment -

        no changes to any component. The Store field is created. and if any component wishes to use it , it can do so.

        Show
        Noble Paul added a comment - no changes to any component. The Store field is created. and if any component wishes to use it , it can do so.
        Hide
        Noble Paul added a comment -

        implemented as Uri said

        Show
        Noble Paul added a comment - implemented as Uri said
        Hide
        Noble Paul added a comment -

        But perhaps we should have gone with the same strategy as the UpdateProcessorChain

        yes, it is. because there are more than one method call per component per request. Should we still try to do it?

        Changing from a method or member to a hash lookup doesn't reduce a dependency when it's needed.

        not true. The fact is that ResponseBuilder stops depending on other components . It is ok for components to depend on ResponseBuilder.

        I believe a Store interface can help here which provide access to data by a key

        Looks better.

        Show
        Noble Paul added a comment - But perhaps we should have gone with the same strategy as the UpdateProcessorChain yes, it is. because there are more than one method call per component per request. Should we still try to do it? Changing from a method or member to a hash lookup doesn't reduce a dependency when it's needed. not true. The fact is that ResponseBuilder stops depending on other components . It is ok for components to depend on ResponseBuilder. I believe a Store interface can help here which provide access to data by a key Looks better.
        Hide
        Yonik Seeley added a comment -

        definitely it looks verbose (ugly) . But the point is where do we stop?

        Where it makes sense. There's no reason for an all or nothing approach. There's no reason not to give some special treatment to some main components.

        If one want's to better decompose some of the state for each handler, I guess there could be objects that represented that too?
        class ResponseBuilder

        { QueryInfo queryInfo; FacetInfo facetInfo; }

        But perhaps we should have gone with the same strategy as the UpdateProcessorChain... an actual object is created for each component to keep the state for the request.

        I believe the public API's should have no dependency on components .

        Changing from a method or member to a hash lookup doesn't reduce a dependency when it's needed.

        Show
        Yonik Seeley added a comment - definitely it looks verbose (ugly) . But the point is where do we stop? Where it makes sense. There's no reason for an all or nothing approach. There's no reason not to give some special treatment to some main components. If one want's to better decompose some of the state for each handler, I guess there could be objects that represented that too? class ResponseBuilder { QueryInfo queryInfo; FacetInfo facetInfo; } But perhaps we should have gone with the same strategy as the UpdateProcessorChain... an actual object is created for each component to keep the state for the request. I believe the public API's should have no dependency on components . Changing from a method or member to a hash lookup doesn't reduce a dependency when it's needed.
        Hide
        Uri Boness added a comment - - edited

        if (rb.store.get(HighlightingComponent.DO_HIGHLIGHTING) == Boolean.TRUE)

        This is verbose... too verbose to my taste. I believe a Store interface can help here which provide access to data by a key and will also provide helper methods to keep the code clean. (a MapStore can be a simple implementation which wraps a Map<String, Object> instance):

        public interface Store {
        
            Boolean getBoolean(String key);
            boolean getBoolean(String key, boolean defaultValue);
            
            Integer getInt(String key);
            int getInt(String key, int defaultValue);
        
            //other methods for all primitive types and dates.
        }
        

        so now you have:

        if (rb.store.getBoolean(HighlightingComponent.DO_HIGHLIGHTING, false))
        

        which is cleaner and is NPE-safe.

        I believe the public API's should have no dependency on components .

        I agree. Basically avoid having circular dependencies. You don't want to change the platform API every time you introduce a new component.

        Show
        Uri Boness added a comment - - edited if (rb.store.get(HighlightingComponent.DO_HIGHLIGHTING) == Boolean.TRUE) This is verbose... too verbose to my taste. I believe a Store interface can help here which provide access to data by a key and will also provide helper methods to keep the code clean. (a MapStore can be a simple implementation which wraps a Map<String, Object> instance): public interface Store { Boolean getBoolean( String key); boolean getBoolean( String key, boolean defaultValue); Integer getInt( String key); int getInt( String key, int defaultValue); //other methods for all primitive types and dates. } so now you have: if (rb.store.getBoolean(HighlightingComponent.DO_HIGHLIGHTING, false )) which is cleaner and is NPE-safe. I believe the public API's should have no dependency on components . I agree. Basically avoid having circular dependencies. You don't want to change the platform API every time you introduce a new component.
        Hide
        Noble Paul added a comment -

        if ((Boolean)rb.store.get(HighlightingComponent.DO_HIGHLIGHTING))

        This may cause NPE

        so it will look like

        if (rb.store.get(HighlightingComponent.DO_HIGHLIGHTING) == Boolean.TRUE)

        definitely it looks verbose (ugly) . But the point is where do we stop? Which all components qualify to be important enough. Is 'my component' more/lessimportant than FacetComponent. If we set a coding standard , isn't that better? I believe the public API's should have no dependency on components .

        Show
        Noble Paul added a comment - if ((Boolean)rb.store.get(HighlightingComponent.DO_HIGHLIGHTING)) This may cause NPE so it will look like if (rb.store.get(HighlightingComponent.DO_HIGHLIGHTING) == Boolean.TRUE) definitely it looks verbose (ugly) . But the point is where do we stop? Which all components qualify to be important enough. Is 'my component' more/lessimportant than FacetComponent. If we set a coding standard , isn't that better? I believe the public API's should have no dependency on components .
        Hide
        Yonik Seeley added a comment -

        This one change can keep the ResponseBuilder from being a kitchen sink

        But we should do it selectively. Replacing a bunch of flags with public strings that one needs to look up doesn't seem to be an improvement.

        if (rb.doHighlighting)
        vs
        if ((Boolean)rb.store.get(HighlightingComponent.DO_HIGHLIGHTING))

        Show
        Yonik Seeley added a comment - This one change can keep the ResponseBuilder from being a kitchen sink But we should do it selectively. Replacing a bunch of flags with public strings that one needs to look up doesn't seem to be an improvement. if (rb.doHighlighting) vs if ((Boolean)rb.store.get(HighlightingComponent.DO_HIGHLIGHTING))
        Hide
        Noble Paul added a comment -

        While we should make it easy for custom components to store their own flags + state, etc, we should be careful about hiding the state of well known components

        By keeping the key public we ensure that other components can see/change it.

        it eliminates a hash lookup for each component each time through the loop

        We do 100s of hash lookups in each request. This one change can keep the ResponseBuilder from being a kitchen sink and the design cleaner .

        Is the plan to have a KEY per component instance? If so, how would it be possible to refer to the key from other components?

        you are right. We should make it public static. I hope that there aren't multiple instances of the same component registered in the same handler

        Show
        Noble Paul added a comment - While we should make it easy for custom components to store their own flags + state, etc, we should be careful about hiding the state of well known components By keeping the key public we ensure that other components can see/change it. it eliminates a hash lookup for each component each time through the loop We do 100s of hash lookups in each request. This one change can keep the ResponseBuilder from being a kitchen sink and the design cleaner . Is the plan to have a KEY per component instance? If so, how would it be possible to refer to the key from other components? you are right. We should make it public static. I hope that there aren't multiple instances of the same component registered in the same handler
        Hide
        Yonik Seeley added a comment -

        While we should make it easy for custom components to store their own flags + state, etc, we should be careful about hiding the state of well known components. It's OK to hide internal implementation stuff, but we should leave anything useful to other components exposed (and more explicitly spell out what it means, if it can be changed by another component, etc).

        Whether a request is a faceting request or not could certainly be useful to other components... and as a bonus it eliminates a hash lookup for each component each time through the loop.

        Show
        Yonik Seeley added a comment - While we should make it easy for custom components to store their own flags + state, etc, we should be careful about hiding the state of well known components. It's OK to hide internal implementation stuff, but we should leave anything useful to other components exposed (and more explicitly spell out what it means, if it can be changed by another component, etc). Whether a request is a faceting request or not could certainly be useful to other components... and as a bonus it eliminates a hash lookup for each component each time through the loop.
        Hide
        Uri Boness added a comment -

        We should not keep it static. public final should be good enough

        Is there a special reason for this? Is the plan to have a KEY per component instance? If so, how would it be possible to refer to the key from other components?

        This is what I had in mind - Assuming Component1 computed something and registered it in the store using KEY. Then Component2 can reuse this computation by accessing it as follows:

        Object someValue = rb.store.get(Component2.KEY);
        // do something with someValue
        
        Show
        Uri Boness added a comment - We should not keep it static. public final should be good enough Is there a special reason for this? Is the plan to have a KEY per component instance? If so, how would it be possible to refer to the key from other components? This is what I had in mind - Assuming Component1 computed something and registered it in the store using KEY. Then Component2 can reuse this computation by accessing it as follows: Object someValue = rb.store.get(Component2.KEY); // do something with someValue
        Hide
        Noble Paul added a comment -

        I guess this can be supported by publishing the key as a public static field.

        We should not keep it static. public final should be good enough

        Show
        Noble Paul added a comment - I guess this can be supported by publishing the key as a public static field. We should not keep it static. public final should be good enough
        Hide
        Uri Boness added a comment -

        It should also be possible to share objects between components to eliminate duplicate computations (if one component can re-use a computation that was already done in another component). I guess this can be supported by publishing the key as a public static field.

        Show
        Uri Boness added a comment - It should also be possible to share objects between components to eliminate duplicate computations (if one component can re-use a computation that was already done in another component). I guess this can be supported by publishing the key as a public static field.
        Hide
        Noble Paul added a comment -

        A new Map is kept is ResponseBuilder. It is also possible to use SolrQueryRequest.getContext(). Let us make that call.

        Every component keeps a private Object as the key and stores values by that key in rb.store . No other component can access that value because the key is a private Object (So no conflict). The patch illustrates how FacetComponent has eliminated the variables from ResponseBuilder. If it is fine , we can remove the rest of it too such as doStats,needDocList,needDocSet etc

        Show
        Noble Paul added a comment - A new Map is kept is ResponseBuilder. It is also possible to use SolrQueryRequest.getContext(). Let us make that call. Every component keeps a private Object as the key and stores values by that key in rb.store . No other component can access that value because the key is a private Object (So no conflict). The patch illustrates how FacetComponent has eliminated the variables from ResponseBuilder. If it is fine , we can remove the rest of it too such as doStats,needDocList,needDocSet etc
        Hide
        Uri Boness added a comment -

        Don't you have it already with the SolrQueryRequest.getContext()

        Show
        Uri Boness added a comment - Don't you have it already with the SolrQueryRequest.getContext()
        Hide
        Noble Paul added a comment -

        Let us keep a Map in RB so that each component can store/retrieve any values it want. It can be considered as a session object .

        Show
        Noble Paul added a comment - Let us keep a Map in RB so that each component can store/retrieve any values it want. It can be considered as a session object .

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Shalin Shekhar Mangar
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development