HBase
  1. HBase
  2. HBASE-11015

Some refactoring on MemStoreFlusher

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Not A Problem
    • Affects Version/s: None
    • Fix Version/s: 0.89-fb
    • Component/s: io
    • Labels:

      Description

      Use `ScheduledThreadPoolExecutor`
      Change some logic
      Add testcase

      1. D1264374.diff.txt
        47 kB
        @deprecated Yi Deng

        Activity

        Hide
        @deprecated Yi Deng added a comment -

        This patch was supposed to fix some bugs introduced by another submit, which tried add function of online change the number of flushing threads.

        Some refactoring jobs are also included in this diff.

        Show
        @deprecated Yi Deng added a comment - This patch was supposed to fix some bugs introduced by another submit, which tried add function of online change the number of flushing threads. Some refactoring jobs are also included in this diff.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12640640/D1264374.diff.txt
        against trunk revision .
        ATTACHMENT ID: 12640640

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 6 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9315//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12640640/D1264374.diff.txt against trunk revision . ATTACHMENT ID: 12640640 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 6 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9315//console This message is automatically generated.
        Hide
        @deprecated Yi Deng added a comment -

        I should not have click on "submit patch" when I wanted to attach a file.

        Show
        @deprecated Yi Deng added a comment - I should not have click on "submit patch" when I wanted to attach a file.
        Hide
        stack added a comment -

        Thanks for the patch Yi Deng.

        Why you making the change? You have numbers or a use case you are improving for or is it just an 'itch'?

        Rather than HRegionIf, maybe call it 'Region'? (See where we have Store and HStore as the implementation).

        No need to put HSTORE_BLOCKING_STORE_FILES_KEY in HConstants? Put into class where it is used in MSFlusher? (HConstants is a bit of anti-pattern – good for constants used in many packages)

        Does this need to be in HRegion as a public method:

        + public int maxStoreFilesCount() {

        Would a utiltiy method do here instead? (Should be called getMaxStoreFileCount?)

        Look at other Interfaces in HBase. See how they do not have the public qualifiers. I bring it up because a kind heart went through all of our Interfaces and removed the 'public' qualifiers intentionally (They are not needed on Interfaces).

        On this:

        HRegionServerIf

        There is a RegionServerServices interface all ready? You didn't want to use that?

        How are these done currently? The Master runs them IIRC?

        + /**
        + * Requests the region server to make a split on a specific region-store.
        + */
        + public boolean requestSplit(HRegionIf r);
        +
        + /**
        + * Requests the region server to make a compaction on a specific region-store.
        + *
        + * @param r the region-store.
        + * @param why Why compaction requested – used in debug messages
        + */
        + public void requestCompaction(HRegionIf r, String why);

        I reviewed about half of the patch.

        Show
        stack added a comment - Thanks for the patch Yi Deng. Why you making the change? You have numbers or a use case you are improving for or is it just an 'itch'? Rather than HRegionIf, maybe call it 'Region'? (See where we have Store and HStore as the implementation). No need to put HSTORE_BLOCKING_STORE_FILES_KEY in HConstants? Put into class where it is used in MSFlusher? (HConstants is a bit of anti-pattern – good for constants used in many packages) Does this need to be in HRegion as a public method: + public int maxStoreFilesCount() { Would a utiltiy method do here instead? (Should be called getMaxStoreFileCount?) Look at other Interfaces in HBase. See how they do not have the public qualifiers. I bring it up because a kind heart went through all of our Interfaces and removed the 'public' qualifiers intentionally (They are not needed on Interfaces). On this: HRegionServerIf There is a RegionServerServices interface all ready? You didn't want to use that? How are these done currently? The Master runs them IIRC? + /** + * Requests the region server to make a split on a specific region-store. + */ + public boolean requestSplit(HRegionIf r); + + /** + * Requests the region server to make a compaction on a specific region-store. + * + * @param r the region-store. + * @param why Why compaction requested – used in debug messages + */ + public void requestCompaction(HRegionIf r, String why); I reviewed about half of the patch.
        Hide
        @deprecated Yi Deng added a comment -

        Stack This is a patch based on HBase version of Facebook internal. I attached the file because someone else like to have a look.

        Show
        @deprecated Yi Deng added a comment - Stack This is a patch based on HBase version of Facebook internal. I attached the file because someone else like to have a look.
        Hide
        Elliott Clark added a comment -

        0.89-fb is no longer being actively maintained. If issues persist open an issue against the current master or stable versions.

        Show
        Elliott Clark added a comment - 0.89-fb is no longer being actively maintained. If issues persist open an issue against the current master or stable versions.

          People

          • Assignee:
            Elliott Clark
            Reporter:
            @deprecated Yi Deng
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development