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 2020/11/19 21:47:44 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1796: Make TabletLocationState human readable
milleruntime opened a new pull request #1796:
URL: https://github.com/apache/accumulo/pull/1796
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1796: Make TabletLocationState human readable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1796:
URL: https://github.com/apache/accumulo/pull/1796#discussion_r527236251
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java
##########
@@ -89,42 +89,39 @@ public String toString() {
public TServerInstance getLocation() {
TServerInstance result = null;
- if (current != null) {
+ if (hasCurrent()) {
result = current;
- } else if (future != null) {
+ } else if (hasFuture()) {
result = future;
} else {
result = last;
}
return result;
}
- private static final int _HAS_CURRENT = 1 << 0;
- private static final int _HAS_FUTURE = 1 << 1;
- private static final int _HAS_SUSPEND = 1 << 2;
+ public boolean hasCurrent() {
+ return current != null;
+ }
+
+ public boolean hasFuture() {
+ return future != null;
+ }
+
+ public boolean hasSuspend() {
+ return suspend != null;
+ }
public TabletState getState(Set<TServerInstance> liveServers) {
- switch ((current == null ? 0 : _HAS_CURRENT) | (future == null ? 0 : _HAS_FUTURE)
- | (suspend == null ? 0 : _HAS_SUSPEND)) {
- case 0:
- return TabletState.UNASSIGNED;
-
- case _HAS_SUSPEND:
- return TabletState.SUSPENDED;
-
- case _HAS_FUTURE:
- case (_HAS_FUTURE | _HAS_SUSPEND):
- return liveServers.contains(future) ? TabletState.ASSIGNED
- : TabletState.ASSIGNED_TO_DEAD_SERVER;
-
- case _HAS_CURRENT:
- case (_HAS_CURRENT | _HAS_SUSPEND):
- return liveServers.contains(current) ? TabletState.HOSTED
- : TabletState.ASSIGNED_TO_DEAD_SERVER;
-
- default:
- // Both current and future are set, which is prevented by constructor.
- throw new IllegalStateException();
- }
+ TabletState state = TabletState.UNASSIGNED;
+ if (hasFuture())
+ state =
+ liveServers.contains(future) ? TabletState.ASSIGNED : TabletState.ASSIGNED_TO_DEAD_SERVER;
+ else if (hasCurrent())
+ state =
+ liveServers.contains(current) ? TabletState.HOSTED : TabletState.ASSIGNED_TO_DEAD_SERVER;
+ else if (hasSuspend())
+ state = TabletState.SUSPENDED;
+
+ return state;
Review comment:
That's funny you prefer the multiple returns because I originally had this but changed it to the local variable thinking it was more readable. Hopefully I can just hit undo a few time in my IDE...
----------------------------------------------------------------
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 #1796: Make TabletLocationState human readable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1796:
URL: https://github.com/apache/accumulo/pull/1796#discussion_r527237800
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java
##########
@@ -89,42 +89,39 @@ public String toString() {
public TServerInstance getLocation() {
TServerInstance result = null;
- if (current != null) {
+ if (hasCurrent()) {
result = current;
- } else if (future != null) {
+ } else if (hasFuture()) {
result = future;
} else {
result = last;
}
return result;
}
- private static final int _HAS_CURRENT = 1 << 0;
- private static final int _HAS_FUTURE = 1 << 1;
- private static final int _HAS_SUSPEND = 1 << 2;
+ public boolean hasCurrent() {
+ return current != null;
+ }
+
+ public boolean hasFuture() {
+ return future != null;
+ }
+
+ public boolean hasSuspend() {
+ return suspend != null;
+ }
public TabletState getState(Set<TServerInstance> liveServers) {
- switch ((current == null ? 0 : _HAS_CURRENT) | (future == null ? 0 : _HAS_FUTURE)
- | (suspend == null ? 0 : _HAS_SUSPEND)) {
- case 0:
- return TabletState.UNASSIGNED;
-
- case _HAS_SUSPEND:
- return TabletState.SUSPENDED;
-
- case _HAS_FUTURE:
- case (_HAS_FUTURE | _HAS_SUSPEND):
- return liveServers.contains(future) ? TabletState.ASSIGNED
- : TabletState.ASSIGNED_TO_DEAD_SERVER;
-
- case _HAS_CURRENT:
- case (_HAS_CURRENT | _HAS_SUSPEND):
- return liveServers.contains(current) ? TabletState.HOSTED
- : TabletState.ASSIGNED_TO_DEAD_SERVER;
-
- default:
- // Both current and future are set, which is prevented by constructor.
- throw new IllegalStateException();
- }
+ TabletState state = TabletState.UNASSIGNED;
+ if (hasFuture())
+ state =
+ liveServers.contains(future) ? TabletState.ASSIGNED : TabletState.ASSIGNED_TO_DEAD_SERVER;
+ else if (hasCurrent())
+ state =
+ liveServers.contains(current) ? TabletState.HOSTED : TabletState.ASSIGNED_TO_DEAD_SERVER;
+ else if (hasSuspend())
+ state = TabletState.SUSPENDED;
+
+ return state;
Review comment:
It's your choice. Either way is correct behavior.
----------------------------------------------------------------
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] milleruntime merged pull request #1796: Make TabletLocationState human readable
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1796:
URL: https://github.com/apache/accumulo/pull/1796
----------------------------------------------------------------
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 #1796: Make TabletLocationState human readable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1796:
URL: https://github.com/apache/accumulo/pull/1796#discussion_r527228654
##########
File path: core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java
##########
@@ -89,42 +89,39 @@ public String toString() {
public TServerInstance getLocation() {
TServerInstance result = null;
- if (current != null) {
+ if (hasCurrent()) {
result = current;
- } else if (future != null) {
+ } else if (hasFuture()) {
result = future;
} else {
result = last;
}
return result;
}
- private static final int _HAS_CURRENT = 1 << 0;
- private static final int _HAS_FUTURE = 1 << 1;
- private static final int _HAS_SUSPEND = 1 << 2;
+ public boolean hasCurrent() {
+ return current != null;
+ }
+
+ public boolean hasFuture() {
+ return future != null;
+ }
+
+ public boolean hasSuspend() {
+ return suspend != null;
+ }
public TabletState getState(Set<TServerInstance> liveServers) {
- switch ((current == null ? 0 : _HAS_CURRENT) | (future == null ? 0 : _HAS_FUTURE)
- | (suspend == null ? 0 : _HAS_SUSPEND)) {
- case 0:
- return TabletState.UNASSIGNED;
-
- case _HAS_SUSPEND:
- return TabletState.SUSPENDED;
-
- case _HAS_FUTURE:
- case (_HAS_FUTURE | _HAS_SUSPEND):
- return liveServers.contains(future) ? TabletState.ASSIGNED
- : TabletState.ASSIGNED_TO_DEAD_SERVER;
-
- case _HAS_CURRENT:
- case (_HAS_CURRENT | _HAS_SUSPEND):
- return liveServers.contains(current) ? TabletState.HOSTED
- : TabletState.ASSIGNED_TO_DEAD_SERVER;
-
- default:
- // Both current and future are set, which is prevented by constructor.
- throw new IllegalStateException();
- }
+ TabletState state = TabletState.UNASSIGNED;
+ if (hasFuture())
+ state =
+ liveServers.contains(future) ? TabletState.ASSIGNED : TabletState.ASSIGNED_TO_DEAD_SERVER;
+ else if (hasCurrent())
+ state =
+ liveServers.contains(current) ? TabletState.HOSTED : TabletState.ASSIGNED_TO_DEAD_SERVER;
+ else if (hasSuspend())
+ state = TabletState.SUSPENDED;
+
+ return state;
Review comment:
Minor nits: not having the final "else" bothers me. Also, you don't need to have the local variable.
```suggestion
if (hasFuture())
return
liveServers.contains(future) ? TabletState.ASSIGNED : TabletState.ASSIGNED_TO_DEAD_SERVER;
else if (hasCurrent())
return
liveServers.contains(current) ? TabletState.HOSTED : TabletState.ASSIGNED_TO_DEAD_SERVER;
else if (hasSuspend())
return TabletState.SUSPENDED;
else
return TabletState.UNASSIGNED;
```
----------------------------------------------------------------
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] milleruntime commented on pull request #1796: Make TabletLocationState human readable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1796:
URL: https://github.com/apache/accumulo/pull/1796#issuecomment-730675914
Running full ITs here: https://github.com/milleruntime/accumulo/actions/runs/373232410
----------------------------------------------------------------
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] milleruntime commented on pull request #1796: Make TabletLocationState human readable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1796:
URL: https://github.com/apache/accumulo/pull/1796#issuecomment-731250543
I saw the periodic failure in #1574 but the IT passed when I ran it again.
----------------------------------------------------------------
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