anmolnar commented on code in PR #1943:
URL: https://github.com/apache/zookeeper/pull/1943#discussion_r1039441532
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/IPAuthenticationProvider.java:
##########
@@ -128,4 +131,18 @@ public boolean isValid(String id) {
return true;
}
+ /**
+ * Returns the HTTP(s) client IP address
+ * @param request HttpServletRequest
+ * @return IP address
+ */
+ public static String getClientIPAddress(final HttpServletRequest request) {
+ // to handle the case that a HTTP(s) client connects via a proxy or
load balancer
+ final String xForwardedForHeader =
request.getHeader(X_FORWARDED_FOR_HEADER_NAME);
+ if (xForwardedForHeader == null) {
+ return request.getRemoteAddr();
+ }
+ // the format of the field is: X-Forwarded-For: client, proxy1, proxy2
...
+ return new StringTokenizer(xForwardedForHeader,
",").nextToken().trim();
+ }
Review Comment:
Fine. Next time please add a refactoring to the upcoming patch instead.
Looking at this patch alone means that you break the encapsulation principle.
##########
zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md:
##########
@@ -1208,7 +1208,32 @@ property, when available, is noted below.
The default value is false.
-
+* *serializeLastProcessedZxid.enabled*
+ (Jave system property: **zookeeper.serializeLastProcessedZxid.enabled**)
Review Comment:
Yeah, it probably doesn't hurt, but we should not implement something just
because it doesn't hurt. Also don't need to prepare feature flags for
unexcpeted issues. These are not valid reasons.
Either way, you can leave it as it is. One more thing: the doc mentions that
this option is required for the Admin's snapshot command to work properly. If
that's the case the Admin command should check whether this option is enabled,
but I don't see the check there, right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
