Uploaded image for project: 'Jackrabbit Oak'
  1. Jackrabbit Oak
  2. OAK-5121

review CommitInfo==null in BackgroundObserver with isExternal change

    Details

    • Type: Task
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 1.5.13
    • Fix Version/s: 1.8
    • Component/s: core
    • Labels:
      None

      Description

      OAK-4898 changes CommitInfo to be never null. This is the case outside of the BackgroundObserver - but in the BackgroundObserver itself it is explicitly set to null when compacting.
      Once OAK-4898 is committed this task is about reviewing the implications in BackgroundObserver wrt compaction and CommitInfo==null

      1. OAK-5121.patch
        10 kB
        Stefan Egli

        Issue Links

          Activity

          Hide
          egli Stefan Egli added a comment -

          Chetan Mehrotra, can I assign this to you? I won't find time for this so soon.

          Show
          egli Stefan Egli added a comment - Chetan Mehrotra , can I assign this to you? I won't find time for this so soon.
          Hide
          egli Stefan Egli added a comment -

          Attaching OAK-5121.patch with an illustration of what I had in mind. Basically to have a new flag isCompacted on the CommitInfo and thus a new CommitInfo.EMPTY_COMPACTED. At the moment the code would be identical as isExternal() is replaced with isExternal() || isCompacted. But we could distinguish these two cases more fine-grained going further, eg also in the stats.

          Show
          egli Stefan Egli added a comment - Attaching OAK-5121.patch with an illustration of what I had in mind. Basically to have a new flag isCompacted on the CommitInfo and thus a new CommitInfo.EMPTY_COMPACTED . At the moment the code would be identical as isExternal() is replaced with isExternal() || isCompacted . But we could distinguish these two cases more fine-grained going further, eg also in the stats.
          Hide
          chetanm Chetan Mehrotra added a comment -

          but perhaps an overflow CommitInfo

          Okie makes sense. Lets then create a marker one as part of this issue and we can open a new issue for optimization around merging ChangeSets

          Show
          chetanm Chetan Mehrotra added a comment - but perhaps an overflow CommitInfo Okie makes sense. Lets then create a marker one as part of this issue and we can open a new issue for optimization around merging ChangeSets
          Hide
          egli Stefan Egli added a comment -

          Chetan Mehrotra I thought perhaps we can make it more explicit in the BackgroundObserver that on overflow it is not really an external CommitInfo, but perhaps an overflow CommitInfo - which 'looks like an external' one but in the code we could distinguish this internally to the BackgroundObserver. I think that's what I had in mind with this ticket. But other than that yes, merging the ChangeSets for all the overflowing changes is required.

          Show
          egli Stefan Egli added a comment - Chetan Mehrotra I thought perhaps we can make it more explicit in the BackgroundObserver that on overflow it is not really an external CommitInfo, but perhaps an overflow CommitInfo - which 'looks like an external' one but in the code we could distinguish this internally to the BackgroundObserver. I think that's what I had in mind with this ticket. But other than that yes, merging the ChangeSets for all the overflowing changes is required.
          Hide
          chetanm Chetan Mehrotra added a comment -

          For OAK-4898 I have change the BackgroundObserver to use CommitInfo.EMPTY_EXTERNAL for commit info when doing compaction of content change. So that part is ok. Pending work there now is to merge ChangeSet when doing such compaction but that is more of optimization

          Stefan Egli Is that understanding correct?

          Show
          chetanm Chetan Mehrotra added a comment - For OAK-4898 I have change the BackgroundObserver to use CommitInfo.EMPTY_EXTERNAL for commit info when doing compaction of content change. So that part is ok. Pending work there now is to merge ChangeSet when doing such compaction but that is more of optimization Stefan Egli Is that understanding correct?
          Hide
          egli Stefan Egli added a comment -

          removed from 1.5.14 as it depends on OAK-4898 which is not yet committed.

          Show
          egli Stefan Egli added a comment - removed from 1.5.14 as it depends on OAK-4898 which is not yet committed.

            People

            • Assignee:
              chetanm Chetan Mehrotra
              Reporter:
              egli Stefan Egli
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development