lmccay commented on code in PR #1049:
URL: https://github.com/apache/knox/pull/1049#discussion_r2130110688
##########
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/CommonHaDispatch.java:
##########
@@ -38,14 +40,17 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;
-public interface LBHaDispatch {
+public interface CommonHaDispatch {
Review Comment:
Not sure this was needed to be renamed. Failover is typically a LB feature
as well. Nothing you need to change but it probably would have required fewer
other changes to use the new name everywhere.
##########
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/CommonHaDispatch.java:
##########
@@ -240,4 +270,73 @@ default void shiftActiveURL(boolean userAgentDisabled,
Optional<URI> backendURI)
}
}
}
+
Review Comment:
It is super confusing to have so much code in an interface.
Maybe only so for old eyes like mine though.
Based on the default modifier being in this class previously, I assume that
you are not introducing it here for the first time. I'll think about it a bit
more further into the review where multiple inheritance starts to rear its
head. :)
##########
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/SSEHaDispatch.java:
##########
@@ -136,17 +149,100 @@ public void setActiveURL(String url) {
activeURL.set(url);
}
+ @Override
+ public int getMaxFailoverAttempts() {
+ return maxFailoverAttempts;
+ }
+
+ @Override
+ public void setMaxFailoverAttempts(int maxFailoverAttempts) {
+ this.maxFailoverAttempts = maxFailoverAttempts;
+ }
+
+ @Override
+ public int getFailoverSleep() {
+ return failoverSleep;
+ }
+
+ @Override
+ public void setFailoverSleep(int failoverSleep) {
+ this.failoverSleep = failoverSleep;
+ }
+
+ @Override
+ public void setFailoverNonIdempotentRequestEnabled(boolean enabled) {
+ this.failoverNonIdempotentRequestEnabled = enabled;
+ }
+
+ @Override
+ public boolean isFailoverNonIdempotentRequestEnabled() {
+ return failoverNonIdempotentRequestEnabled;
+ }
+
+ @Override
+ public void setNoFallbackEnabled(boolean enabled) {
+ this.noFallbackEnabled = enabled;
+ }
+
+ @Override
+ public boolean isNoFallbackEnabled() {
+ return noFallbackEnabled;
+ }
+
Review Comment:
Is this only because SSEHaDispatch doesn't extend the common one?
##########
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/SSEHaDispatch.java:
##########
@@ -136,17 +149,100 @@ public void setActiveURL(String url) {
activeURL.set(url);
}
+ @Override
+ public int getMaxFailoverAttempts() {
+ return maxFailoverAttempts;
+ }
+
+ @Override
+ public void setMaxFailoverAttempts(int maxFailoverAttempts) {
+ this.maxFailoverAttempts = maxFailoverAttempts;
+ }
+
+ @Override
+ public int getFailoverSleep() {
+ return failoverSleep;
+ }
+
+ @Override
+ public void setFailoverSleep(int failoverSleep) {
+ this.failoverSleep = failoverSleep;
+ }
+
+ @Override
+ public void setFailoverNonIdempotentRequestEnabled(boolean enabled) {
+ this.failoverNonIdempotentRequestEnabled = enabled;
+ }
+
+ @Override
+ public boolean isFailoverNonIdempotentRequestEnabled() {
+ return failoverNonIdempotentRequestEnabled;
+ }
+
+ @Override
+ public void setNoFallbackEnabled(boolean enabled) {
+ this.noFallbackEnabled = enabled;
+ }
+
+ @Override
+ public boolean isNoFallbackEnabled() {
+ return noFallbackEnabled;
+ }
+
Review Comment:
All of the above methods have to be duplicated across all implementers of
CommonHaDispatch? Why is that? If they were added to a common extension instead
wouldn't that be better?
--
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: dev-unsubscr...@knox.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
