-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-1651. Create a http.policy config for Ozone #930
Conversation
Change-Id: Ia284f685f6d39a512124e6055537615d325ae96b
💔 -1 overall
This message was automatically generated. |
@@ -64,6 +64,8 @@ | |||
"dfs.container.ratis.ipc"; | |||
public static final int DFS_CONTAINER_RATIS_IPC_PORT_DEFAULT = 9858; | |||
|
|||
public static final String OZONE_HTTP_POLICY = "ozone.http.policy"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This newly added property should be used during HTTP server start.
That part is missing.
@@ -426,6 +430,20 @@ public static long getUtcTime() { | |||
return Calendar.getInstance(UTC_ZONE).getTimeInMillis(); | |||
} | |||
|
|||
public static Policy getHttpPolicy(Configuration conf) { | |||
String policyStr = conf.get("ozone.http.policy", OzoneConfigKeys.OZONE_HTTP_POLICY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor NIT: ozone.http.policy -> Use OzoneConfigKeys.OZONE_HTTP_POLICY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line get() second param value should be a default value for the property.
Change-Id: I047aafc733c936fb82f926bcde489595cf51d928
if (policy == null) { | ||
throw new HadoopIllegalArgumentException("Unrecognized value '" + policyStr + "' for " + "dfs.http.policy"); | ||
} else { | ||
conf.set("dfs.http.policy", policy.name()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set it back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You @shwetayakkali for the contribution.
I have a few comments.
String policyStr = conf.get("ozone.http.policy", OzoneConfigKeys.OZONE_HTTP_POLICY); | ||
if(policyStr == null || policyStr.length() == 0) { | ||
policyStr = conf.get("dfs.http.policy", DFSConfigKeys.DFS_HTTP_POLICY_DEFAULT); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If HTTP_ONLY is used as the default,
we can conf.get("OzoneConfigKeys.OZONE_HTTP_POLICY", DFSConfigKeys.DFS_HTTP_POLICY_DEFAULT);
Then we don't need checks of null, and we can make this logic simple.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
One more comment, we need to change the code in OzoneManagerSnapShotProvider class, to use this newly added method. Line 89: this.httpPolicy = DFSUtil.getHttpPolicy(conf); |
Change-Id: I23b1ddc619488bb290be9136a18ec5e2aab7dd77
💔 -1 overall
This message was automatically generated. |
Hi, I feel ozone can also re-use the hdfs config. In this way, we don't miss the code changes where HTTP policy is being used, like OzoneManagerSnapShotProvider which uses still DFSUtils.getHttpPolicy. And also we use to create HttpServer2.Builder builder with below code. That again uses dfs.http.policy. So, this is the reason we need to set ozone.http.policy to dfs.http.policy to make it work. So, I feel to avoid all these, we can use dfs.http.policy as before. Let me know your thoughts on this.
|
@@ -31,6 +31,7 @@ | |||
import java.util.Optional; | |||
import java.util.TimeZone; | |||
|
|||
import org.apache.hadoop.HadoopIllegalArgumentException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Unused import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for using dfs.http..policy to reduce redundant config knobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You @eyanghwx for review.
I also feel the same. Can we close this as won't Fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You @eyanghwx for the confirmation.
I will close this as Won't fix.
Thank You @shwetayakkali for the contribution. |
Change-Id: Ia284f685f6d39a512124e6055537615d325ae96b