You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ddanielr (via GitHub)" <gi...@apache.org> on 2023/04/19 15:07:59 UTC
[GitHub] [accumulo] ddanielr opened a new pull request, #3321: Add ProblemReport section to metadata schema
ddanielr opened a new pull request, #3321:
URL: https://github.com/apache/accumulo/pull/3321
Adds ProblemReport section to the metadata schema to consolidate metadata table prefixes.
This removes hardcoded prefix strings located in various classes.
--
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 pull request #3321: Add ProblemReport section to metadata schema
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3321:
URL: https://github.com/apache/accumulo/pull/3321#issuecomment-1522363662
Thanks, @ddanielr . This can be merged once CI checks are finished.
--
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 merged pull request #3321: Add ProblemReport section to metadata schema
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3321:
URL: https://github.com/apache/accumulo/pull/3321
--
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] ddanielr commented on pull request #3321: Add ProblemReport section to metadata schema
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #3321:
URL: https://github.com/apache/accumulo/pull/3321#issuecomment-1516893266
> I wonder if some of these APIs have a String equivalent, so we don't have to wrap them all with `new Text`. That could be another slight improvement that would go nicely along with this change (if they are equivalent).
That's a great idea! I just added the changes to remove the Text wrappers.
Mutation already had a CharSequence method signature to use. I didn't think we needed to add a method for Strings, but happy to do so if that's desired.
I also had to modify the Encoding util to support a byte array for full removal of the Hadoop Text wrappers.
This change seemed minimal as Encoding was only being used by ProblemReport and its test class and immediately generated a byte array from the Text object.
--
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] ddanielr commented on a diff in pull request #3321: Add ProblemReport section to metadata schema
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #3321:
URL: https://github.com/apache/accumulo/pull/3321#discussion_r1176534419
##########
server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java:
##########
@@ -217,9 +218,9 @@ private Iterator<Entry<Key,Value>> getIter2() {
scanner.setTimeout(3, TimeUnit.SECONDS);
if (table == null) {
- scanner.setRange(new Range(new Text("~err_"), false, new Text("~err`"), false));
+ scanner.setRange(ProblemSection.getRange());
} else {
- scanner.setRange(new Range(new Text("~err_" + table)));
+ scanner.setRange(new Range(new Text(ProblemSection.getRowPrefix() + table)));
Review Comment:
I found two more instances of `new Text` use in ProblemReports and was able to remove all three and the import `Text` statement.
--
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 diff in pull request #3321: Add ProblemReport section to metadata schema
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3321:
URL: https://github.com/apache/accumulo/pull/3321#discussion_r1175924473
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java:
##########
@@ -401,6 +401,23 @@ public static String getRowPrefix() {
}
+ /**
+ * Holds error message processing flags
+ */
+ public static class ProblemSection {
+ private static final Section section =
+ new Section(RESERVED_PREFIX + "err_", true, RESERVED_PREFIX + "err`", false);
Review Comment:
This section definition includes the underscore as a mandatory part of the section. That's the most narrowly scoped definition, and completely fine. However, I'm a bit surprised that the section wasn't defined more widely as everything starting with `err`, as in something like:
```java
new Section(RESERVED_PREFIX + "err", true, RESERVED_PREFIX + "ers", false);
```
I am not suggesting changing it... just noting the distinction between the narrow section definition and the wider definition.
##########
server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java:
##########
@@ -217,9 +218,9 @@ private Iterator<Entry<Key,Value>> getIter2() {
scanner.setTimeout(3, TimeUnit.SECONDS);
if (table == null) {
- scanner.setRange(new Range(new Text("~err_"), false, new Text("~err`"), false));
+ scanner.setRange(ProblemSection.getRange());
} else {
- scanner.setRange(new Range(new Text("~err_" + table)));
+ scanner.setRange(new Range(new Text(ProblemSection.getRowPrefix() + table)));
Review Comment:
This can avoid a Text:
```suggestion
scanner.setRange(new Range(ProblemSection.getRowPrefix() + table));
```
##########
server/base/src/main/java/org/apache/accumulo/server/problems/ProblemReports.java:
##########
@@ -217,9 +218,9 @@ private Iterator<Entry<Key,Value>> getIter2() {
scanner.setTimeout(3, TimeUnit.SECONDS);
if (table == null) {
- scanner.setRange(new Range(new Text("~err_"), false, new Text("~err`"), false));
+ scanner.setRange(ProblemSection.getRange());
Review Comment:
One difference is that the new version has the start key inclusive, and the old range has it exclusive. I don't think this matters, since we don't expect any rows to just include the prefix without a tableId after the underscore, so this isn't an important change. I'm just noting it here as a very slight difference in behavior, in case there's a surprise later, and this comment serves to help debug something.
--
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