You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/06/24 15:40:25 UTC
[GitHub] [accumulo] foster33 opened a new pull request #2180: Update CheckForMetadataProblems Utility Output
foster33 opened a new pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180
Fixes #2166
This PR updates the output of the CheckForMetadataProblems utility. It previously included a mix of table ids and names.
The output now offers more information and is much more easily read.
Some changes and additions that are included:
- `checkTable` function now accepts a String tableName
- Uses `Tables.getTableID()` and `Tables.getTableName()` to get information about the specific tables
- Places the client scanner inside of the try block in `checkMetadataAndRootTableEntries()`
- Updates potential error messages that were not shown in the output in the initial ticket.
During my testing I did notice that there is an extra line that shows some log info about the path to accumulo.properties that gets displayed before the expected output. I am not sure if this is a big issue that it is displayed, but if there is a way to not have it displayed that would probably be best.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 merged pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
Manno15 merged pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#discussion_r658119653
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -125,7 +128,12 @@ private static void checkMetadataAndRootTableEntries(String tableNameToCheck, Se
TreeSet<KeyExtent> tablets = tables.get(tableId);
if (tablets == null) {
- tables.forEach(CheckForMetadataProblems::checkTable);
+ for (Entry<TableId,TreeSet<KeyExtent>> e : tables.entrySet()) {
+ TableId key = e.getKey();
+ TreeSet<KeyExtent> value = e.getValue();
+ String tableName = Tables.getTableName(opts.getServerContext(), key);
+ checkTable(key, value, tableName);
+ }
Review comment:
I think this line could have stayed the same and the name lookup could have occurred in the method. Also, need to handle the case that looking up the name fails, in case the table is deleted, but there is still metadata left in the metadata tablet. That wasn't an issue before because we only used the table ID before.
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -90,19 +92,20 @@ private static void checkTable(TableId tableId, TreeSet<KeyExtent> tablets) {
lastEndRow = tabke.endRow();
}
if (everythingLooksGood)
- System.out.println("All is well for table " + tableId);
+ System.out.println("...All is well for table " + tableName + " (" + tableId + ")");
else
sawProblems = true;
}
private static void checkMetadataAndRootTableEntries(String tableNameToCheck, ServerUtilOpts opts)
throws Exception {
- System.out.println("Checking table: " + tableNameToCheck);
+ TableId tableCheckId = Tables.getTableId(opts.getServerContext(), tableNameToCheck);
+ System.out.println("Checking tables whos metadata is found in: " + tableNameToCheck + " ("
Review comment:
```suggestion
System.out.println("Checking tables whose metadata is found in: " + tableNameToCheck + " ("
```
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -90,19 +92,20 @@ private static void checkTable(TableId tableId, TreeSet<KeyExtent> tablets) {
lastEndRow = tabke.endRow();
}
if (everythingLooksGood)
- System.out.println("All is well for table " + tableId);
+ System.out.println("...All is well for table " + tableName + " (" + tableId + ")");
Review comment:
If these "All is well" messages are indented with `...`, then the error messages should also be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#issuecomment-867744305
Can you give a sample of the output. Similar to what would be shown in the tools section on the accumulo-website.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] foster33 commented on pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
foster33 commented on pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#issuecomment-867765440
```
The updated output:
> accumulo org.apache.accumulo.server.util.CheckForMetadataProblems -u root
2021-06-24T12:03:47,299 [conf.SiteConfiguration] INFO : Found Accumulo configuration on classpath at /home/lukefoster/github/fluo-uno/install/accumulo-2.1.0-SNAPSHOT/conf/accumulo.properties
Checking tables whos metadata is found in: accumulo.root (+r)
...All is well for table accumulo.metadata (!0)
No problems found in accumulo.root (+r)
Checking tables whos metadata is found in: accumulo.metadata (!0)
...All is well for table accumulo.replication (+rep)
...All is well for table trace (1)
No problems found in accumulo.metadata (!0)
```
This also shows the extra log line I had mentioned in the initial comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#discussion_r659997601
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -125,7 +128,12 @@ private static void checkMetadataAndRootTableEntries(String tableNameToCheck, Se
TreeSet<KeyExtent> tablets = tables.get(tableId);
if (tablets == null) {
- tables.forEach(CheckForMetadataProblems::checkTable);
+ for (Entry<TableId,TreeSet<KeyExtent>> e : tables.entrySet()) {
+ TableId key = e.getKey();
+ TreeSet<KeyExtent> value = e.getValue();
+ String tableName = Tables.getTableName(opts.getServerContext(), key);
+ checkTable(key, value, tableName);
+ }
Review comment:
> Is there any way that I would easily be able to recreate this error in testing?
You could put the `Tables.getTableName` in a different method, and then do a partial mock or subclass this utility to provide a different response. However, it's not critical. Just wrapping `Tables.getTableName` with a try-catch block and setting tableName to null if there's an exception should be sufficient, as long as none of the rest of the code minds it being null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#discussion_r664750324
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -75,12 +86,12 @@ private static void checkTable(TableId tableId, TreeSet<KeyExtent> tablets) {
KeyExtent tabke = tabIter.next();
boolean broke = false;
if (tabke.prevEndRow() == null) {
- System.out
- .println("Table " + tableId + " has null prev end row in middle of table " + tabke);
+ System.out.println("Table " + tableName + " (" + tableId
Review comment:
```suggestion
System.out.println("...Table " + tableName + " (" + tableId
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] foster33 commented on a change in pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#discussion_r660602708
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -125,7 +128,12 @@ private static void checkMetadataAndRootTableEntries(String tableNameToCheck, Se
TreeSet<KeyExtent> tablets = tables.get(tableId);
if (tablets == null) {
- tables.forEach(CheckForMetadataProblems::checkTable);
+ for (Entry<TableId,TreeSet<KeyExtent>> e : tables.entrySet()) {
+ TableId key = e.getKey();
+ TreeSet<KeyExtent> value = e.getValue();
+ String tableName = Tables.getTableName(opts.getServerContext(), key);
+ checkTable(key, value, tableName);
+ }
Review comment:
This should be addressed in my latest commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#discussion_r659997601
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -125,7 +128,12 @@ private static void checkMetadataAndRootTableEntries(String tableNameToCheck, Se
TreeSet<KeyExtent> tablets = tables.get(tableId);
if (tablets == null) {
- tables.forEach(CheckForMetadataProblems::checkTable);
+ for (Entry<TableId,TreeSet<KeyExtent>> e : tables.entrySet()) {
+ TableId key = e.getKey();
+ TreeSet<KeyExtent> value = e.getValue();
+ String tableName = Tables.getTableName(opts.getServerContext(), key);
+ checkTable(key, value, tableName);
+ }
Review comment:
> Is there any way that I would easily be able to recreate this error in testing?
You could put the `Tables.getTableName` in a different method, and then do a partial mock or subclass this utility to provide a different response. However, it's not critical. Just wrapping `Tables.getTableName` with a try-catch block and setting tableName to null if there's an exception should be sufficient, as long as none of the rest of the code minds it being null.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] foster33 commented on a change in pull request #2180: Update CheckForMetadataProblems Utility Output
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2180:
URL: https://github.com/apache/accumulo/pull/2180#discussion_r658177554
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -125,7 +128,12 @@ private static void checkMetadataAndRootTableEntries(String tableNameToCheck, Se
TreeSet<KeyExtent> tablets = tables.get(tableId);
if (tablets == null) {
- tables.forEach(CheckForMetadataProblems::checkTable);
+ for (Entry<TableId,TreeSet<KeyExtent>> e : tables.entrySet()) {
+ TableId key = e.getKey();
+ TreeSet<KeyExtent> value = e.getValue();
+ String tableName = Tables.getTableName(opts.getServerContext(), key);
+ checkTable(key, value, tableName);
+ }
Review comment:
>Also, need to handle the case that looking up the name fails, in case the table is deleted, but there is still metadata left in the metadata tablet
Is there any way that I would easily be able to recreate this error in testing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org