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/11/08 16:27:32 UTC
[GitHub] [accumulo] AlbertWhitlock opened a new pull request #2349: Fixes Accumulo #2207
AlbertWhitlock opened a new pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349
Fixes Accumulo #2207. Improves logging message and comments in code.
--
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 #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r754764088
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1433,8 +1432,10 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
all.remove(extent);
if (!all.isEmpty()) {
- log.error("Tablet {} overlaps previously assigned {} {} {}", extent,
- unopenedOverlapping, openingOverlapping, onlineOverlapping + " " + all);
+ log.error(
+ "Tablet {} overlaps a previously assigned tablet. It is possibly due to a recent split. "
+ + "Possible overlapping tablets: Unopened: {}, Opening: {}, Online: {}",
+ extent, unopenedOverlapping, openingOverlapping, onlineOverlapping);
Review comment:
I think these are definitely overlapping, not just possibly overlapping
```suggestion
+ "Overlapping tablets: Unopened: {}, Opening: {}, Online: {}",
extent, unopenedOverlapping, openingOverlapping, onlineOverlapping);
```
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1433,8 +1432,10 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
all.remove(extent);
if (!all.isEmpty()) {
- log.error("Tablet {} overlaps previously assigned {} {} {}", extent,
- unopenedOverlapping, openingOverlapping, onlineOverlapping + " " + all);
+ log.error(
+ "Tablet {} overlaps a previously assigned tablet. It is possibly due to a recent split. "
Review comment:
```suggestion
"Tablet {} overlaps a previously assigned tablet, possibly due to a recent split. "
```
--
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] milleruntime commented on a change in pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r750581288
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1433,8 +1432,11 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
all.remove(extent);
if (!all.isEmpty()) {
- log.error("Tablet {} overlaps previously assigned {} {} {}", extent,
- unopenedOverlapping, openingOverlapping, onlineOverlapping + " " + all);
+ log.error(
+ "Tablet {} overlaps a previously assigned tablet. It is possibly due to a recent split. "
+ + "Possible overlapping tablets:\n" + "Unopened tablet: {}\n"
+ + "Opening tablet: {}\n" + "Online tablet: {}",
Review comment:
I think it is generally good practice to keep the log message to one line. Some tools may expect log messages to be on one line. If you did that, you could shorten the message a lot, not having to worry about including extra spaces and when you list the "Possible overlapping tablets" you don't need print "tablet" for each type.
--
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] Manno15 commented on a change in pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r745024461
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1433,8 +1432,11 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
all.remove(extent);
if (!all.isEmpty()) {
- log.error("Tablet {} overlaps previously assigned {} {} {}", extent,
- unopenedOverlapping, openingOverlapping, onlineOverlapping + " " + all);
+ log.error("Tablet {} overlaps a previously assigned tablet. It is possibly due to a recent split " +
+ "(though it tries to ignore them as a reason to throw this error.) Possible overlapping tablets:\n" +
+ "Unopened tablet: {}\n" +
+ "Opening tablet: {}\n" +
+ "Online tablet: {}", extent, unopenedOverlapping, openingOverlapping, onlineOverlapping);
Review comment:
```suggestion
log.error("Tablet {} overlaps a previously assigned tablet. It is possibly due to a recent split. " +
"Possible overlapping tablets:\n" +
"Unopened tablet: {}\n" +
"Opening tablet: {}\n" +
"Online tablet: {}", extent, unopenedOverlapping, openingOverlapping, onlineOverlapping);
```
I am not quite sure what the parenthesis section was adding and I don't think it is necessary to this log message.
--
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] DomGarguilo commented on a change in pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r744933536
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1433,8 +1431,11 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
all.remove(extent);
if (!all.isEmpty()) {
- log.error("Tablet {} overlaps previously assigned {} {} {}", extent,
- unopenedOverlapping, openingOverlapping, onlineOverlapping + " " + all);
+ log.error("Tablet {} overlaps a previously assigned tablet. It is possibly due to a "
+ + "recent split (though it tries to ignore them as a reason to throw this error.) ",
+ extent + "Possible overlapping tablets:\n" + "Unopened tablet: {}\n",
+ unopenedOverlapping + "Opening tablet: {}\n",
+ openingOverlapping + "Online tablet: {}\n", onlineOverlapping);
Review comment:
I don't think this will print out how I think you intend it to. This might be closer to what you want:
```suggestion
log.error(
"Tablet {} overlaps a previously assigned tablet. It is possibly due to a recent split. "
+ "Possible overlapping tablets:\nUnopened tablet: {}\nOpening tablet: {}"
+ "\nOnline tablet: {}",
extent, unopenedOverlapping, openingOverlapping, onlineOverlapping);
```
With my changes, the output will look like this:
```
ERROR: Tablet EXTENT overlaps a previously assigned tablet. It is possibly due to a recent split. Possible overlapping tablets:
Unopened tablet: UNOPENED
Opening tablet: OPENING
Online tablet: ONLINE
```
--
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] milleruntime merged pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349
--
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] DomGarguilo commented on a change in pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r744933536
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1433,8 +1431,11 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
all.remove(extent);
if (!all.isEmpty()) {
- log.error("Tablet {} overlaps previously assigned {} {} {}", extent,
- unopenedOverlapping, openingOverlapping, onlineOverlapping + " " + all);
+ log.error("Tablet {} overlaps a previously assigned tablet. It is possibly due to a "
+ + "recent split (though it tries to ignore them as a reason to throw this error.) ",
+ extent + "Possible overlapping tablets:\n" + "Unopened tablet: {}\n",
+ unopenedOverlapping + "Opening tablet: {}\n",
+ openingOverlapping + "Online tablet: {}\n", onlineOverlapping);
Review comment:
I don't think this will print out how I think you intend it to. This might be closer to what you want:
```suggestion
log.error(
"Tablet {} overlaps a previously assigned tablet. It is possibly due to a recent split. "
+ "Possible overlapping tablets:\nUnopened tablet: {}\nOpening tablet: {}"
+ "\nOnline tablet: {}",
extent, unopenedOverlapping, openingOverlapping, onlineOverlapping);
```
With my changes, the output will look like this:
```
ERROR: Tablet EXTENT overlaps a previously assigned tablet. It is possibly due to a recent split. Possible overlapping tablets:
Unopened tablet: UNOPENED
Opening tablet: OPENING
Online tablet: ONLINE
```
Edit: you will probably have to re-format if you include these changes.
--
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] Manno15 commented on a change in pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r744957763
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1402,10 +1402,8 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
synchronized (server.openingTablets) {
synchronized (server.onlineTablets) {
- // checking if this exact tablet is in any of the sets
- // below is not a strong enough check
- // when splits and fix splits occurring
-
+ // With splits occurring, checking if the current tablet
+ // is in any of the sets below may not be a strong enough check
Review comment:
```suggestion
// Checking if the current tablet is in any of the sets
// below is not a strong enough check to catch all overlapping tablets
// when splits and fix splits are occurring
```
With the changed to the error message, I think the original comment along with the small change here is enough of an improvement.
--
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] Manno15 commented on a change in pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r744947838
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1402,10 +1402,8 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
synchronized (server.openingTablets) {
synchronized (server.onlineTablets) {
- // checking if this exact tablet is in any of the sets
- // below is not a strong enough check
- // when splits and fix splits occurring
-
+ // With splits occurring, checking if the current tablet
+ // is in any of the sets below may not be a strong enough check
Review comment:
This is mostly just a reorder of works of the original comment. I think it would help to mention what the checks are doing, i.e. checking if the current tablet overlaps any other in the set.
--
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] Manno15 commented on a change in pull request #2349: Improve logging message and comments if tablet overlap occurs.
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2349:
URL: https://github.com/apache/accumulo/pull/2349#discussion_r744947838
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java
##########
@@ -1402,10 +1402,8 @@ public void loadTablet(TInfo tinfo, TCredentials credentials, String lock,
synchronized (server.openingTablets) {
synchronized (server.onlineTablets) {
- // checking if this exact tablet is in any of the sets
- // below is not a strong enough check
- // when splits and fix splits occurring
-
+ // With splits occurring, checking if the current tablet
+ // is in any of the sets below may not be a strong enough check
Review comment:
This is mostly just a reorder of words of the original comment. I think it would help to mention what the checks are doing, i.e. checking if the current tablet overlaps any other in the set.
--
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