Commons Configuration
  1. Commons Configuration
  2. CONFIGURATION-180

Cache DatabaseConfiguration values for higher performance

    Details

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

      Description

      The DatabaseConfiguration class queries each property as it gets the request for each property. This is
      nice for ensuring that you're always up to date, but it doesn't give very good performance in enterprise
      applications, where the database may not be on the same subnet.
      What we need is the ability to hit the database once, get all the keys/values and then serve up
      "getString" etc. from that cache.
      I'll be opening a separate enhancement to have a generic Reloading Strategy which could then be
      applied to this caching DatabaseConfiguration approach.

        Issue Links

          Activity

          Hide
          Emmanuel Bourg added a comment -

          I see several solutions to improve the performances by caching the properties. We may either :

          1. load the entire configuration in memory. A flush() method would save the new/modified properties back to the database, a sync() method would reload the properties. That may be nice to reuse the reloading strategies but currently they are specific to the file configurations, that will require some refactoring.

          2. keep in memory the properties read for a certain time. getProperty("foo") will hit the database only if foo was read more than 5 minutes ago for example. The delay would be adjustable. Setting a property would still hit the database directly.

          3. same as 2, but the configuration is preloaded once.

          The caching should be optional to preserve the current behavior if needed.

          Show
          Emmanuel Bourg added a comment - I see several solutions to improve the performances by caching the properties. We may either : 1. load the entire configuration in memory. A flush() method would save the new/modified properties back to the database, a sync() method would reload the properties. That may be nice to reuse the reloading strategies but currently they are specific to the file configurations, that will require some refactoring. 2. keep in memory the properties read for a certain time. getProperty("foo") will hit the database only if foo was read more than 5 minutes ago for example. The delay would be adjustable. Setting a property would still hit the database directly. 3. same as 2, but the configuration is preloaded once. The caching should be optional to preserve the current behavior if needed.
          Hide
          Emmanuel Bourg added a comment -

          I modified DatabaseConfiguration to ensure the connection is always closed.

          Show
          Emmanuel Bourg added a comment - I modified DatabaseConfiguration to ensure the connection is always closed.
          Hide
          Oliver Siegmar added a comment -

          Created an attachment (id=14708)
          Optional preloading

          I originally sent this patch to the ML because I didn't know of this bugzilla
          entry. Emmanuel Bourg asked for appending the patch here. My patch adds
          database preloading optionally and changes less code of DatabaseConfiguration
          'internals'. It doesn't fix the bug in closeQuitely() either. I think the user
          should have the choice to enable/disable preloading (non-enterprise
          applications with a big configuration table might benefit from not preloading
          all configuration).

          Show
          Oliver Siegmar added a comment - Created an attachment (id=14708) Optional preloading I originally sent this patch to the ML because I didn't know of this bugzilla entry. Emmanuel Bourg asked for appending the patch here. My patch adds database preloading optionally and changes less code of DatabaseConfiguration 'internals'. It doesn't fix the bug in closeQuitely() either. I think the user should have the choice to enable/disable preloading (non-enterprise applications with a big configuration table might benefit from not preloading all configuration).
          Hide
          Stephen Cooper added a comment -

          I'm working on a refresh approach like what is done for properties. With that, then the cache will
          always be at most x seconds older than the database, so my hope is that the querying the cache will be
          sufficient. What I might do is take and refactor out the real logic of isEmpty etc to the strategey. Have
          one strategy which always queries the database, one strategy which uses a cache and refreshes etc.
          The downside there is it kinda kills my idea of having a generic reloading strategy which could be used
          for xml, properties files and database. Perhaps that's just a pipe dream.

          As far as the finals go, that's something I always do with instance variables which are set in the
          constructor and then never change. It has two advantages:
          1) compiler makes sure that I initialize the fields before use and
          2) compiler can apply optimizations (inining, direct memory access etc.)
          Also, with the default settings for checkstyle which I've kinda gotten ingrained in me now, checkstyle
          recommends setting those puppies to final.

          Show
          Stephen Cooper added a comment - I'm working on a refresh approach like what is done for properties. With that, then the cache will always be at most x seconds older than the database, so my hope is that the querying the cache will be sufficient. What I might do is take and refactor out the real logic of isEmpty etc to the strategey. Have one strategy which always queries the database, one strategy which uses a cache and refreshes etc. The downside there is it kinda kills my idea of having a generic reloading strategy which could be used for xml, properties files and database. Perhaps that's just a pipe dream. As far as the finals go, that's something I always do with instance variables which are set in the constructor and then never change. It has two advantages: 1) compiler makes sure that I initialize the fields before use and 2) compiler can apply optimizations (inining, direct memory access etc.) Also, with the default settings for checkstyle which I've kinda gotten ingrained in me now, checkstyle recommends setting those puppies to final.
          Hide
          Emmanuel Bourg added a comment -

          Adding a cache to DatabaseConfiguration is a good idea. However I'm not sure I
          would rely solely on the cache for some operations like isEmpty() or
          containsKey(). The cache should be first checked, and then the database should
          be queried if necessary. Checking if the cache is empty isn't enough to make
          sure the configuration is empty.

          Just out of curiosity, why did you change the private members to final ?

          Show
          Emmanuel Bourg added a comment - Adding a cache to DatabaseConfiguration is a good idea. However I'm not sure I would rely solely on the cache for some operations like isEmpty() or containsKey(). The cache should be first checked, and then the database should be queried if necessary. Checking if the cache is empty isn't enough to make sure the configuration is empty. Just out of curiosity, why did you change the private members to final ?
          Hide
          Stephen Cooper added a comment -

          Created an attachment (id=14277)
          Patch to implement this. This one checks out clean in checkstyle.

          Show
          Stephen Cooper added a comment - Created an attachment (id=14277) Patch to implement this. This one checks out clean in checkstyle.
          Hide
          Stephen Cooper added a comment -

          Created an attachment (id=14276)
          Patch to implement this.

          This patch, in addition to implementing the caching DB behavior, also corrects
          a minor bug in closeQuietly, where a SQLException in the stmnt.close() call
          would prevent the connection from ever being closed.

          Show
          Stephen Cooper added a comment - Created an attachment (id=14276) Patch to implement this. This patch, in addition to implementing the caching DB behavior, also corrects a minor bug in closeQuietly, where a SQLException in the stmnt.close() call would prevent the connection from ever being closed.

            People

            • Assignee:
              Emmanuel Bourg
              Reporter:
              Stephen Cooper
            • Votes:
              3 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development