reiabreu commented on code in PR #9169:
URL: https://github.com/apache/storm/pull/9169#discussion_r4303177305
##########
storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java:
##########
@@ -1258,9 +1258,6 @@ static Map<String, Object> normalizeConf(Map<String,
Object> conf, Map<String, O
ret.put(Config.TOPOLOGY_WORKER_NIMBUS_THRIFT_CLIENT_USE_TLS,
workerNimbusClientTlsEnabled);
ret.put(Config.NIMBUS_THRIFT_CLIENT_USE_TLS,
workerNimbusClientTlsEnabled);
- if (!mergedConf.containsKey(Config.TOPOLOGY_METRICS_REPORTERS) &&
mergedConf.containsKey(Config.STORM_METRICS_REPORTERS)) {
- ret.put(Config.TOPOLOGY_METRICS_REPORTERS,
mergedConf.get(Config.STORM_METRICS_REPORTERS));
- }
// add any system metrics reporters to the topology metrics reporters
Review Comment:
Removing the fallback above means a `storm.metrics.reporters` that is still
set in storm.yaml is silently ignored. A warning would tell operators what
happened (optional):
```suggestion
if (mergedConf.containsKey("storm.metrics.reporters")) {
LOG.warn("storm.metrics.reporters was removed and is ignored;
use topology.metrics.reporters instead");
}
// add any system metrics reporters to the topology metrics reporters
```
##########
storm-client/src/jvm/org/apache/storm/Config.java:
##########
@@ -2640,39 +2615,19 @@ private static String
substituteHostnameInPrincipal(String principal) throws Unk
return principal;
}
- @Deprecated
- @SuppressWarnings("checkstyle:AbbreviationAsWordInName")
- public static String getBlobstoreHDFSPrincipal(Map conf) throws
UnknownHostException {
- return getHdfsPrincipal(conf);
- }
-
/**
* Get the hostname substituted hdfs principal.
*
* @param conf the storm Configuration
* @return the principal
- * @throws UnknownHostException on UnknowHostException
+ * @throws UnknownHostException on UnknownHostException
*/
public static String getHdfsPrincipal(Map<String, Object> conf) throws
UnknownHostException {
- String ret;
-
- String blobstorePrincipal = (String)
conf.get(Config.BLOBSTORE_HDFS_PRINCIPAL);
String hdfsPrincipal = (String)
conf.get(Config.STORM_HDFS_LOGIN_PRINCIPAL);
- if (blobstorePrincipal == null && hdfsPrincipal == null) {
+ if (hdfsPrincipal == null) {
Review Comment:
The old warning that pointed operators to `storm.hdfs.login.principal` goes
away with the key. An operator who set only `blobstore.hdfs.principal` now gets
`null` here and no message, and the keytab path behaves the same way. A warning
for one release would help (optional; the keytab method would need the same):
```suggestion
if (hdfsPrincipal == null) {
if (conf.get("blobstore.hdfs.principal") != null) {
LOG.warn("blobstore.hdfs.principal was removed and is
ignored; use storm.hdfs.login.principal instead");
}
```
--
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]
