Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.7.0
    • Component/s: None
    • Labels:
      None

      Description

      The tablet constructor leaks this when it calls tabletResources.setTablet(this, acuTableConf); AFAIK this is completey innocuous because nothing is really done (in 1.4 code) w/ the reference. Howerver, it is possible the code could be changed in the future to use the ref.

        Issue Links

          Activity

          Hide
          Bill Havanki added a comment -

          I'll try this one. I've been meaning to get into the Tablet code.

          Show
          Bill Havanki added a comment - I'll try this one. I've been meaning to get into the Tablet code.
          Hide
          Josh Elser added a comment -

          I've been meaning to get into the Tablet code.

          Famous last words

          (just kidding, of course)

          Show
          Josh Elser added a comment - I've been meaning to get into the Tablet code. Famous last words (just kidding, of course)
          Hide
          Bill Havanki added a comment -

          Where is the Like button on JIRA?

          Show
          Bill Havanki added a comment - Where is the Like button on JIRA?
          Hide
          John Vines added a comment -

          you can vote on tickets, that's close!

          Show
          John Vines added a comment - you can vote on tickets, that's close!
          Hide
          Bill Havanki added a comment -

          First shot at this up for review. I still plan to test this out on an actual cluster.

          Show
          Bill Havanki added a comment - First shot at this up for review. I still plan to test this out on an actual cluster.
          Hide
          Bill Havanki added a comment -

          Third attempt up for review. After testing I found an issue with the simpler second attempt. Notably, this latest attempt changes the memory management framework for tablets slightly, so that minor compaction is not attempted on an offline tablet. This makes sense in my limited understanding of things, so others' opinions are definitely needed. See the review for more details.

          Show
          Bill Havanki added a comment - Third attempt up for review. After testing I found an issue with the simpler second attempt. Notably, this latest attempt changes the memory management framework for tablets slightly, so that minor compaction is not attempted on an offline tablet. This makes sense in my limited understanding of things, so others' opinions are definitely needed. See the review for more details.
          Hide
          Bill Havanki added a comment -

          Attempt #4 up for review. This avoids the online check from the third attempt and also tries to handle the scenario when analysis of a later-unloaded tablet leads to minor compaction of a new, reloaded instance of the same tablet.

          Show
          Bill Havanki added a comment - Attempt #4 up for review. This avoids the online check from the third attempt and also tries to handle the scenario when analysis of a later-unloaded tablet leads to minor compaction of a new, reloaded instance of the same tablet.
          Hide
          Keith Turner added a comment -

          I noticed there is another place in the tablet constructor that leaks this, the call to {{

          {tabletServer.recover(...,this,...)}

          }}. I took a quick look at how its used, it seems to just use the extent and table config.

          Show
          Keith Turner added a comment - I noticed there is another place in the tablet constructor that leaks this, the call to {{ {tabletServer.recover(...,this,...)} }}. I took a quick look at how its used, it seems to just use the extent and table config.
          Hide
          ASF subversion and git services added a comment -

          Commit eade3b59ddd95732cbfc635eadf6447c6b3108d2 in branch refs/heads/master from Bill Havanki
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=eade3b5 ]

          ACCUMULO-1948 Tablet constructor no longer leaks this

          The core Tablet constructor no longer sets this on the TabletResourceManager passed to it.
          The TabletResourceManager instance is created now without a Tablet reference, but only
          with its key extent and configuration, and so it can be constructed first and safely passed
          to the Tablet constructor. This alteration in TabletResourceManager drove changes to several
          other locations which had been expecting a tablet reference to be available. In most of those
          places, the key extent takes its place.

          However, Keith Turner pointed out that, without a tablet reference in the TabletStateImpl
          class (memory report) used to guide minor compaction, it's possible that a report logged
          against a tablet that was since unloaded could cause compaction to start on a new instance
          of that tablet that had just been reloaded. In order to ensure that cannot happen, the
          tablet reference remains, so that the same object is used to decide on compaction and
          potentially perform compaction.

          In addition, the method of working with the memory reports was changed so that the same
          snapshot of them used to decide on compactions is used to find the tablet to compact later.
          This change was necessary regardless of this ticket.

          Finally, the unused tabletResources field was removed from TabletServerResourceManager,
          and that class also now avoids erroneously removing the memory report for a reloaded tablet
          if its prior incarnation is unloaded in the scenario described above.

          Show
          ASF subversion and git services added a comment - Commit eade3b59ddd95732cbfc635eadf6447c6b3108d2 in branch refs/heads/master from Bill Havanki [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=eade3b5 ] ACCUMULO-1948 Tablet constructor no longer leaks this The core Tablet constructor no longer sets this on the TabletResourceManager passed to it. The TabletResourceManager instance is created now without a Tablet reference, but only with its key extent and configuration, and so it can be constructed first and safely passed to the Tablet constructor. This alteration in TabletResourceManager drove changes to several other locations which had been expecting a tablet reference to be available. In most of those places, the key extent takes its place. However, Keith Turner pointed out that, without a tablet reference in the TabletStateImpl class (memory report) used to guide minor compaction, it's possible that a report logged against a tablet that was since unloaded could cause compaction to start on a new instance of that tablet that had just been reloaded. In order to ensure that cannot happen, the tablet reference remains, so that the same object is used to decide on compaction and potentially perform compaction. In addition, the method of working with the memory reports was changed so that the same snapshot of them used to decide on compactions is used to find the tablet to compact later. This change was necessary regardless of this ticket. Finally, the unused tabletResources field was removed from TabletServerResourceManager, and that class also now avoids erroneously removing the memory report for a reloaded tablet if its prior incarnation is unloaded in the scenario described above.
          Hide
          Bill Havanki added a comment -

          Thanks again for the reviews, Keith. I will open a new JIRA for the additional this leakage you located yesterday, since this one got complicated.

          Show
          Bill Havanki added a comment - Thanks again for the reviews, Keith. I will open a new JIRA for the additional this leakage you located yesterday, since this one got complicated.
          Hide
          ASF subversion and git services added a comment -

          Commit dcc19ccbada8c2f0a206ec797455294015e8ca6d in accumulo's branch refs/heads/master from Christopher Tubbs
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=dcc19cc ]

          ACCUMULO-1961 Re-apply inadvertently dropped 4abb3f1 to master branch

          Fixes trivial warnings and broken javadocs which have been recently
          introduced. Specifically, removes references to private and
          package-private (default) classes in public javadoc comments (internal
          details aren't relevant to the API and subject to change). Another
          common warning was unused imports and javadoc param tags that refer to
          non-existent parameters.

          Commits against the following JIRA issues introduced these:
          ACCUMULO-1948, ACCUMULO-1974, ACCUMULO-2021, ACCUMULO-2136,
          ACCUMULO-2322, ACCUMULO-2334, ACCUMULO-2350

          Show
          ASF subversion and git services added a comment - Commit dcc19ccbada8c2f0a206ec797455294015e8ca6d in accumulo's branch refs/heads/master from Christopher Tubbs [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=dcc19cc ] ACCUMULO-1961 Re-apply inadvertently dropped 4abb3f1 to master branch Fixes trivial warnings and broken javadocs which have been recently introduced. Specifically, removes references to private and package-private (default) classes in public javadoc comments (internal details aren't relevant to the API and subject to change). Another common warning was unused imports and javadoc param tags that refer to non-existent parameters. Commits against the following JIRA issues introduced these: ACCUMULO-1948 , ACCUMULO-1974 , ACCUMULO-2021 , ACCUMULO-2136 , ACCUMULO-2322 , ACCUMULO-2334 , ACCUMULO-2350

            People

            • Assignee:
              Bill Havanki
              Reporter:
              Keith Turner
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development