Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.8.1.2
    • Fix Version/s: 10.8.1.2
    • Component/s: SQL, Store
    • Labels:
      None

      Description

      Tracks the task of adding the core daemon code, which will be responsible to do the heavy lifting for the istat feature.
      Note that the code will be left disabled, enabling it will be done under a separate Jira.

      1. derby-4936-1b-core_istat_daemon.diff
        72 kB
        Kristian Waagan
      2. derby-4936-1a-core_istat_daemon.stat
        0.5 kB
        Kristian Waagan
      3. derby-4936-1a-core_istat_daemon.diff
        68 kB
        Kristian Waagan

        Issue Links

          Activity

          Hide
          Kristian Waagan added a comment -

          Closing issue.

          Show
          Kristian Waagan added a comment - Closing issue.
          Hide
          Kristian Waagan added a comment -

          Initial core code added. Bugs and required changes will be addressed under separate JIRAs.

          Show
          Kristian Waagan added a comment - Initial core code added. Bugs and required changes will be addressed under separate JIRAs.
          Hide
          Kristian Waagan added a comment -

          Committed patch 1b to trunk with revision 1044413.
          I will wait a few days before I commit DERBY-4937 to go through a few days of automated testing to be sure this code didn't cause any trouble.

          You may see a new line in your derby.log file when shutting down your database, but it will always report zero or false for everything. If you want to suppress it, specify derby.storage.indexStats.log=false.
          I have enabled logging by default now during initial testing to make sure there is some information available about what the daemon is doing in case problems occur.

          Show
          Kristian Waagan added a comment - Committed patch 1b to trunk with revision 1044413. I will wait a few days before I commit DERBY-4937 to go through a few days of automated testing to be sure this code didn't cause any trouble. You may see a new line in your derby.log file when shutting down your database, but it will always report zero or false for everything. If you want to suppress it, specify derby.storage.indexStats.log=false. I have enabled logging by default now during initial testing to make sure there is some information available about what the daemon is doing in case problems occur.
          Hide
          Kristian Waagan added a comment -

          First, I added logic to shutdown on too many consecutive errors (too many set to 50) in 1a (different from DERBY-4771).

          Attaching patch 1b, with the following changes from revision a:

          A java/engine/org/apache/derby/impl/services/daemon/IndexStatisticsDaemonImpl.java

          o Rewrote/added some comments.
          o Removed "utilization rate" and just printed the time spent working and the age of the daemon.
          o Require log stream to be non-null (IllegalArgumentException)
          o Rewrote retry-logic in generateStatistics.
          o Folded method updateAllIndexesStats into generateStatistics (because I had to move invalidateStatements)
          o Changed order of actions in writeUpdatedStats. It will no do:

          • setHeapRowEstimate
          • invalidateStatements
          • dropStats
          • addStats
            o Made writeUpdatedStat return also for background task if numRows == zero.
            o Fancier retry logic in invalidateStatements.
            o Moved some code related to lcc/tx initialization
            o Call ContextManager.cleanupOnError for fatal errors, and some changes to the error handling flow.
            o Destroy tx when stopping (if idle).
            o Added debug method extratIstatInfo(StandardException), which will print to lines of the stack for known errors. It will print the last line of code invoked in the daemon together with the next class/method called. This allows us to better understand why/where an expected error was thrown without logging the full stack.
            o I kept STORAGE_AUTO_INDEX_STATS_DEBUG_ABSDIFF_THRESHOLD, but set the default to zero. Makes it easy to work around problems caused by disabling the check (i.e., set it to a non-zero value).

          I will commit this patch now.

          Show
          Kristian Waagan added a comment - First, I added logic to shutdown on too many consecutive errors (too many set to 50) in 1a (different from DERBY-4771 ). Attaching patch 1b, with the following changes from revision a: A java/engine/org/apache/derby/impl/services/daemon/IndexStatisticsDaemonImpl.java o Rewrote/added some comments. o Removed "utilization rate" and just printed the time spent working and the age of the daemon. o Require log stream to be non-null (IllegalArgumentException) o Rewrote retry-logic in generateStatistics. o Folded method updateAllIndexesStats into generateStatistics (because I had to move invalidateStatements) o Changed order of actions in writeUpdatedStats. It will no do: setHeapRowEstimate invalidateStatements dropStats addStats o Made writeUpdatedStat return also for background task if numRows == zero. o Fancier retry logic in invalidateStatements. o Moved some code related to lcc/tx initialization o Call ContextManager.cleanupOnError for fatal errors, and some changes to the error handling flow. o Destroy tx when stopping (if idle). o Added debug method extratIstatInfo(StandardException), which will print to lines of the stack for known errors. It will print the last line of code invoked in the daemon together with the next class/method called. This allows us to better understand why/where an expected error was thrown without logging the full stack. o I kept STORAGE_AUTO_INDEX_STATS_DEBUG_ABSDIFF_THRESHOLD, but set the default to zero. Makes it easy to work around problems caused by disabling the check (i.e., set it to a non-zero value). I will commit this patch now.
          Hide
          Kristian Waagan added a comment -

          Attaching patch 1a, which adds the core daemon code for the istat feature. For some more discussion, see DERBY-4771.

          This patch is not ready for commit, as there are some issues left to address. Note that to test this feature now, you have to apply the patch(es) for DERBY-4938 and set derby.storage.indexStats.auto=true. This will most likely change after DERBY-4939 has been resolved, since the feature should be enabled by default

          Remaining issues (copied from DERBY-4771, I have given them names):

          A Yes, but not at this level. I thought it would be nice to have a name for the
          transaction to identify it in the transaction table.
          I think some new methods must be added to be able to name a transaction at
          this level, so I'm not sure if it is worth the trouble.
          I'm keeping the TODO for now, might remove it in the next iteration.

          Opinions?

          B It isn't needed now. Since the interface is internal, and there is only one
          implementation of it, I suppose the best action to take now is to remove it.
          We can introduce it again later, and then probably in a shape more like you
          have described. It feels a bit odd to say in the JavaDoc that scheduling
          requests may be denied, and not have a way to learn if it happened or not...

          Opinions?

          C Added synchronization for runningThread in the finally-block.
          The current code will let the thread die and then create a new one on the
          next update request. I considered adding a sleep before letting the thread
          die, in case a new request would come in quickly.

          Opinions?

          D Do you mean we should call TransactionResourceImpl#handleException explicitly
          here?
          I think the comment meant to say that the daemon will be disabled elsewhere.
          I'll address this issue in the next iteration.

          Further review is welcome!

          Show
          Kristian Waagan added a comment - Attaching patch 1a, which adds the core daemon code for the istat feature. For some more discussion, see DERBY-4771 . This patch is not ready for commit, as there are some issues left to address. Note that to test this feature now, you have to apply the patch(es) for DERBY-4938 and set derby.storage.indexStats.auto=true. This will most likely change after DERBY-4939 has been resolved, since the feature should be enabled by default Remaining issues (copied from DERBY-4771 , I have given them names): A Yes, but not at this level. I thought it would be nice to have a name for the transaction to identify it in the transaction table. I think some new methods must be added to be able to name a transaction at this level, so I'm not sure if it is worth the trouble. I'm keeping the TODO for now, might remove it in the next iteration. Opinions? B It isn't needed now. Since the interface is internal, and there is only one implementation of it, I suppose the best action to take now is to remove it. We can introduce it again later, and then probably in a shape more like you have described. It feels a bit odd to say in the JavaDoc that scheduling requests may be denied, and not have a way to learn if it happened or not... Opinions? C Added synchronization for runningThread in the finally-block. The current code will let the thread die and then create a new one on the next update request. I considered adding a sleep before letting the thread die, in case a new request would come in quickly. Opinions? D Do you mean we should call TransactionResourceImpl#handleException explicitly here? I think the comment meant to say that the daemon will be disabled elsewhere. I'll address this issue in the next iteration. Further review is welcome!

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Kristian Waagan
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development