Hbase 22567 by wchevreuil · Pull Request #3 · apache/hbase-operator-tools

wchevreuil · 2019-06-11T17:29:01Z

First PR for addMissingRegionsToMeta

asfgit · 2019-06-11T17:29:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-HBASE-OPERATOR-TOOLS-Build/4/

joshelser · 2019-06-12T15:33:25Z

    return EXIT_FAILURE;
  }

+  int addMissingRegionsInMeta(String... tableNames) throws  IOException {


Could we break this apart into two steps?

Report missing regions in meta

Add a region to meta

We would be able to compose this higher-level tool then, in something like:

REGIONS=$(hbase hbck2 report_missing_regions) for region in REGIONS; do hbase hbck2 add_region "region" done

I think this would help keep the complexity of this method down, making it easier to test as a by-product. It would also let an admin inspect the output from the first step (making sure we want to re-create all of those regions, maybe doing some external validation) before trying to re-create them all in one step.

I think the tricky part would be coming up with the "format" to take the output from step1 and pass it to step2 with minimal "massaging".

WDYT about that, Wellington?

useful for backgound IMHO is git's "plumbing" vs "porcelain":

https://git-scm.com/book/en/v2/Git-Internals-Plumbing-and-Porcelain

analogous here would be "report missing" and "add region" as plumbing level. The output of either need not be user friendly and should focus on being maintainable. (so like encoded region name for the output of report missing would be fine)

something like the wrapper Josh mentions would be a porcelain command, probably taking the encoded regions from report missing and providing a user friendly info (like table, region keys, etc). it'd be useful for it to have a --dryrun mode that skipped calling add_region so that operators could get the more user friendly version of info about what regions appear to be missing.

@joshelser @busbey , Sorry for the delay. I think these are all good ideas. Had pushed a commit where I'm trying to implement the plumbing/porcelain approach within 3 methods: addMissingRegionsInMeta, addMissingRegionsInMetaForTables and reportTablesWithMissingRegionsInMeta. The original porcelain method is now addMissingRegionsInMetaForTables. It combines addMissingRegionsInMeta and reportTablesWithMissingRegionsInMeta calls to add regions back in meta. reportTablesWithMissingRegionsInMeta can is also exposed as a CLI method, as a mean to provide a reporting only command for operators. Some additional UTs are still pending, but an initial manual testing on a broke cluster did look promising. Let me know what you guys think about these, while I will add more UTs for those.

…in order to retrieve threads execution errors

xcangCRM · 2019-06-20T00:20:54Z

+                  try {
+                    admin.disableTable(tableName);
+                  } catch (IOException e) {
+                    LOG.warn("Failed to disable table {}, "


why is it safe to proceed even failing to disable the table?

We try to disable mostly to make sure we will not be dealing with regions that are transient, say if split/merge is happening while we scan meta. Assumption here is that if disable fails, the table is already somehow offline. That would be the case when even namespace table has missing regions, for example, so we need to proceed with regions re-insertion even if we are not able to disable table.

xcangCRM · 2019-06-20T00:25:15Z

+    List<RegionInfo> regionInfos = MetaTableAccessor.
+      getTableRegions(this.conn, tableName, false);
+    for(final FileStatus regionDir : regionsDirs){
+      if(!regionDir.getPath().getName().equals(".tabledesc")&&!regionDir.getPath().getName().equals(".tmp")) {


I spotted some minor formatting issues and also can we put ".tabledesc" into a constant? thanks.
nit:
how about moving regionFoundInMeta() into a separate function that we can unit test?

I spotted some minor formatting issues and also can we put ".tabledesc" into a constant?

Indeed, moving it together with ".tmp" into constants. Will be available in next commit.

how about moving regionFoundInMeta() into a separate function that we can unit test?

I think it's already getting tested by the available tests for findMissingRegionsInMETA method. Since there's not any other client method requiring such logic, I didn't feel it should deserve its own separate method.

tabledesc not already in a constant over in hbase?

Spacing ... need spaces around operators as we have in rest of codebase.

Yeah, is the .tmp defined already too?

Is there not a filitering mechanism elsewhere to rule out these exceptions? Could it be reused?

tabledesc not already in a constant over in hbase?

I could only find a package private constant in FSTableDescriptors, so couldn't reuse it here.

Spacing ... need spaces around operators as we have in rest of codebase.

Addressed on current PR version.

Yeah, is the .tmp defined already too?

Yep, found several constants defined for it, decided to use HConstants.HBASE_TEMP_DIRECTORY on current PR version.

Is there not a filitering mechanism elsewhere to rule out these exceptions? Could it be reused?

I'm not aware of any, but maybe there is. Any ideas on where about in the code to to look for? Tried review some of the hbase-server util package classes, but no luck.

…to smaller pieces following the plumbing/porcelain approach

…rator-tools into HBASE-22567

asfgit · 2019-07-01T18:12:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-HBASE-OPERATOR-TOOLS-Build/5/

saintstack

Nice work

saintstack · 2019-07-02T21:43:28Z

    return EXIT_FAILURE;
  }

+  Map<String,List<Path>> reportTablesWithMissingRegionsInMeta(String... nameSpaceOrTable)


We've done the String that could be one of two things in the past and it proved more trouble than the seeming simplicity it promises. Something to watch out for.

What if operator mixes table and namespace in the list passed?

It would still list missing regions specific for each namespace/table passed as parameter. Even if we pass a namespace and then a table within that namespace, output would show the missing regions grouped. For example, say namespace ns1 has two tables tbl1 and tbl2, and each of these two tables has a missing region r1 and r2, respectively. Then calling reportTablesWithMissingRegionsInMeta ns1 ns1:tbl1, should print:
ns1 -> r1 r2
ns1:tbl1 -> r1

saintstack · 2019-07-02T22:52:23Z

+    List<RegionInfo> regionInfos = MetaTableAccessor.
+      getTableRegions(this.conn, tableName, false);
+    for(final FileStatus regionDir : regionsDirs){
+      if(!regionDir.getPath().getName().equals(".tabledesc")&&!regionDir.getPath().getName().equals(".tmp")) {


tabledesc not already in a constant over in hbase?

saintstack · 2019-07-02T22:53:00Z

+    List<RegionInfo> regionInfos = MetaTableAccessor.
+      getTableRegions(this.conn, tableName, false);
+    for(final FileStatus regionDir : regionsDirs){
+      if(!regionDir.getPath().getName().equals(".tabledesc")&&!regionDir.getPath().getName().equals(".tmp")) {


Spacing ... need spaces around operators as we have in rest of codebase.

Yeah, is the .tmp defined already too?

saintstack · 2019-07-02T22:53:37Z

+    List<RegionInfo> regionInfos = MetaTableAccessor.
+      getTableRegions(this.conn, tableName, false);
+    for(final FileStatus regionDir : regionsDirs){
+      if(!regionDir.getPath().getName().equals(".tabledesc")&&!regionDir.getPath().getName().equals(".tmp")) {


Is there not a filitering mechanism elsewhere to rule out these exceptions? Could it be reused?

saintstack · 2019-07-02T22:54:58Z

+  }
+
+  public void putRegionInfoFromHdfsInMeta(Path region) throws IOException {
+    RegionInfo info = HRegionFileSystem.loadRegionInfoFileContent(fs, region);


Should there be a check we don't overlap with an existing region?

Good point. My first idea was to just leave it break, and operator would need to take further actions, such as merge overlapping regions. We could add additional check, as a mean to warn operator that extra merge would be required later.

Any other thoughts?

tabledesc not already in a constant over in hbase?

I could only find a package private constant in FSTableDescriptors, so couldn't reuse it here.

Spacing ... need spaces around operators as we have in rest of codebase.

Addressed on current PR version.

Yeah, is the .tmp defined already too?

Yep, found several constants defined for it, decided to use HConstants.HBASE_TEMP_DIRECTORY on current PR version.

Is there not a filitering mechanism elsewhere to rule out these exceptions? Could it be reused?

I'm not aware of any, but maybe there is. Any ideas on where about in the code to to look for? Tried review some of the hbase-server util package classes, but no luck.

asfgit · 2019-07-04T17:25:44Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-HBASE-OPERATOR-TOOLS-Build/6/

asfgit · 2019-07-05T18:29:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-HBASE-OPERATOR-TOOLS-Build/7/

busbey · 2019-07-12T03:55:09Z

+    writer.println("   no matches in META, it reads regioninfo metadata file and ");
+    writer.println("   re-creates given region in META. Regions are re-created in 'CLOSED' ");
+    writer.println("   state at META table only, but not in Masters' cache, and are not ");
+    writer.println("   assigned either. A rolling Masters restart, followed by a ");


We should add one that hbck2 can call?

…ing method

asf-ci · 2019-07-15T21:49:46Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-HBASE-OPERATOR-TOOLS-Build/29/

wchevreuil · 2019-07-15T21:53:16Z

Had pushed a new commit d40cfb1, addressing latest suggestions and adding more UTs.

… to the one from reportMissing)

asf-ci · 2019-07-17T16:10:27Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-HBASE-OPERATOR-TOOLS-Build/41/

saintstack · 2019-07-26T18:02:41Z

Looking at this again, because I'm looking at how to have hbck2 plug holes. This is a hole plugger but only for the case where there is a region in HDFS that has been dropped. This should run first. Lets get it in (smile). There are outstanding comments. Maybe take a look?

busbey · 2019-08-06T13:16:45Z

I'd also like to get this landed as a part of getting a released artifact out for hbase-operator-tools soon. I'll keep an eye out for the update, but if I miss it and you're looking for reviews please ping me.

wchevreuil

Thanks for all the insights @saintstack @busbey . Had pushed another commit addressing some styling issues. I had now revisit all outstanding comments. Most of those had been addressed, but for some, I had replied either with some explanation or with alternative solutions. Let me know on your thoughts about current state for this.

wchevreuil · 2019-08-29T15:49:19Z

Had started a new PR after rebasing these changes with latest master version:
#18

Am closing this one.

wchevreuil and others added 5 commits June 7, 2019 17:35

temp commit

477807e

Temp commit

a6f267e

HBASE-22567 - HBCK2 addMissingRegionsToMeta

da6d12f

HBASE-22567 - HBCK2 addMissingRegionsToMeta

3309503

Merge branch 'HBASE-22567' of https://github.com/wchevreuil/hbase-ope…

40a08d9

…rator-tools into HBASE-22567

joshelser reviewed Jun 12, 2019

View reviewed changes

Comment thread hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java Outdated

wchevreuil and others added 5 commits June 19, 2019 11:33

temp commit

4f0e499

Temp commit

026adce

HBASE-22567 - HBCK2 addMissingRegionsToMeta

8070637

HBASE-22567 - HBCK2 addMissingRegionsToMeta

e2349c5

Addressing joshelser's suggestions for switching to futures/callable …

7054bdc

…in order to retrieve threads execution errors

xcangCRM reviewed Jun 20, 2019

View reviewed changes

Comment thread hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java Outdated

xcangCRM reviewed Jun 20, 2019

View reviewed changes

Comment thread hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java Outdated

xcangCRM reviewed Jun 20, 2019

View reviewed changes

wchevreuil added 2 commits July 1, 2019 18:53

addressing suggestions for breaking single addRegionsInMeta method in…

658bfc0

…to smaller pieces following the plumbing/porcelain approach

Merge branch 'HBASE-22567' of https://github.com/wchevreuil/hbase-ope…

209e770

…rator-tools into HBASE-22567

daisukebe reviewed Jul 2, 2019

View reviewed changes

Comment thread hbase-hbck2/src/main/java/org/apache/hbase/hbck2/meta/MetaFixer.java Outdated

saintstack reviewed Jul 2, 2019

View reviewed changes

wchevreuil added 2 commits July 3, 2019 20:11

changes addressing comments as replied on latest pull request.

94e6f46

some more fixes addressing PR review comments

527683a

More review suggestions addressing

9672380

busbey mentioned this pull request Jul 12, 2019

HBASE-22680 [HBCK2] OfflineMetaRepair for hbase2/hbck2 #5

Closed

busbey reviewed Jul 12, 2019

View reviewed changes

busbey reviewed Jul 15, 2019

View reviewed changes

Comment thread hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java Outdated

busbey reviewed Jul 15, 2019

View reviewed changes

Comment thread hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java Outdated

addressing PR suggestions, additional tests and fix to result formatt…

d40cfb1

…ing method

additional fix for addMissingRegions output formatter method (similar…

2773bdf

… to the one from reportMissing)

wchevreuil commented Aug 13, 2019

View reviewed changes

wchevreuil closed this Aug 29, 2019

wchevreuil mentioned this pull request Sep 3, 2019

HBASE-22567 - HBCK2 addMissingRegionsToMeta #18

Closed

Conversation

wchevreuil commented Jun 11, 2019

Uh oh!

asfgit commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchevreuil Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wchevreuil Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jul 1, 2019

Uh oh!

Uh oh!

saintstack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Jul 4, 2019

Uh oh!

asfgit commented Jul 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wchevreuil Aug 13, 2019 •

edited

Loading

wchevreuil Aug 13, 2019 •

edited

Loading