[
https://issues.apache.org/jira/browse/HADOOP-15708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16638707#comment-16638707
]
Zoltan Siegl commented on HADOOP-15708:
---------------------------------------
So to make things a bit clear about the change:
Before the patch:
{code:java}
private String[] handleDeprecation(DeprecationContext deprecations,
String name) {
if (null != name) {
name = name.trim();
}
// Initialize the return value with requested name
String[] names = new String[]{name};
// Deprecated keys are logged once and an updated names are returned
DeprecatedKeyInfo keyInfo = deprecations.getDeprecatedKeyMap().get(name);
if (keyInfo != null) {
if (!keyInfo.getAndSetAccessed()) {
logDeprecation(keyInfo.getWarningMessage(name));
}
// Override return value for deprecated keys
names = keyInfo.newKeys;
}
// **** WE RETURN EARLY HERE IF THERE ARE NO OVERLAYS ***
// If there are no overlay values we can return early
Properties overlayProperties = getOverlay();
if (overlayProperties.isEmpty()) {
return names;
}
// Update properties and overlays with reverse lookup values
for (String n : names) {
String deprecatedKey = deprecations.getReverseDeprecatedKeyMap().get(n);
if (deprecatedKey != null && !overlayProperties.containsKey(n)) {
String deprecatedValue = overlayProperties.getProperty(deprecatedKey);
if (deprecatedValue != null) {
// **** STILL AT THIS LINE WE ARE UPDATING PROPS *NOT* OVERLAY ***
getProps().setProperty(n, deprecatedValue);
overlayProperties.setProperty(n, deprecatedValue);
}
}
}
return names;
}
{code}
So I have extracted the latter loop to run for all props not just overlays as
it should work.
To address concerns of [~jlowe] as we talked offline:
- You said that if you set deprecated and new keys as well, this patch would
change the return value. To address this fear I have created a test case for
this:
{code:java}
@Test
public void testBothPropertiesAreSet() throws Exception {
// SETUP
final String oldZkAddressKey = "yarn.resourcemanager.zk-address";
final String newZkAddressKey = CommonConfigurationKeys.ZK_ADDRESS;
final String oldZkAddressValue = "oldZkAddress";
final String newZkAddressValue = "newZkAddress";
try{
out = new BufferedWriter(new FileWriter(CONFIG4));
startConfig();
appendProperty(oldZkAddressKey, oldZkAddressValue);
appendProperty(newZkAddressKey, newZkAddressValue);
endConfig();
Path fileResource = new Path(CONFIG4);
conf.addResource(fileResource);
} finally {
out.close();
}
// ACT
conf.get(oldZkAddressKey);
Configuration.addDeprecations(new Configuration.DeprecationDelta[] {
new Configuration.DeprecationDelta(oldZkAddressKey, newZkAddressKey)});
// ASSERT
assertEquals("New property should overwrite deprecated one",
newZkAddressValue, conf.get(oldZkAddressKey));
assertEquals("Property should be accessible through new key",
newZkAddressValue, conf.get(newZkAddressKey));
}
{code}
This tests setting both properties and returning the same values before and
after the patch as well.
I did not add this to the patch, as this has been already covered in the
previous test cases, it is juts a bit more readable this way, as a separate
test.
To address fears of [~rkanter]:
- You said that some may depend on the bugous functionality. That is normally a
good idea to stay backward compatible, however why would you depend on getting
NULL instead of the config value you needed? This is a bug. Don'd depend on
bugs.
Is there anything else I can do for this to be accepted? I'd be happy to.
> Reading values from Configuration before adding deprecations make it
> impossible to read value with deprecated key
> -----------------------------------------------------------------------------------------------------------------
>
> Key: HADOOP-15708
> URL: https://issues.apache.org/jira/browse/HADOOP-15708
> Project: Hadoop Common
> Issue Type: Bug
> Components: conf
> Affects Versions: 3.2.0
> Reporter: Szilard Nemeth
> Assignee: Zoltan Siegl
> Priority: Minor
> Attachments: HADOOP-15708-testcase.patch, HADOOP-15708.001.patch,
> HADOOP-15708.002.patch, HADOOP-15708.003.patch
>
>
> Hadoop Common contains a widely used Configuration class.
> This class can handle deprecations of properties, e.g. if property 'A' gets
> deprecated with an alternative property key 'B', users can access property
> values with keys 'A' and 'B'.
> Unfortunately, this does not work in one case.
> When a config file is specified (for instance, XML) and a property is read
> with the config.get() method, the config is loaded from the file at this
> time.
> If the deprecation mapping is not yet specified by the time any config value
> is retrieved and the XML config refers to a deprecated key, then the
> deprecation mapping specified, the config value cannot be retrieved neither
> with the deprecated nor with the new key.
> The attached patch contains a testcase that reproduces this wrong behavior.
> Here are the steps outlined what the testcase does:
> 1. Creates an XML config file with a deprecated property
> 2. Adds the config to the Configuration object
> 3. Retrieves the config with its deprecated key (it does not really matter
> which property the user gets, could be any)
> 4. Specifies the deprecation rules including the one defined in the config
> 5. Prints and asserts the property retrieved from the config with both the
> deprecated and the new property keys.
> For reference, here is the log of one execution that actually shows what the
> issue is:
> {noformat}
> Loaded items: 1
> Looked up property value with name hadoop.zk.address: null
> Looked up property value with name yarn.resourcemanager.zk-address:
> dummyZkAddress
> Contents of config file: [<?xml version="1.0"?>, <configuration>,
> <property><name>yarn.resourcemanager.zk-address</name><value>dummyZkAddress</value></property>,
> </configuration>]
> Looked up property value with name hadoop.zk.address: null
> 2018-08-31 10:10:06,484 INFO Configuration.deprecation
> (Configuration.java:logDeprecation(1397)) - yarn.resourcemanager.zk-address
> is deprecated. Instead, use hadoop.zk.address
> Looked up property value with name hadoop.zk.address: null
> Looked up property value with name hadoop.zk.address: null
> java.lang.AssertionError:
> Expected :dummyZkAddress
> Actual :null
> {noformat}
> *As it's visible from the output and the code, the issue is really that if
> the config is retrieved either with the deprecated or the new value,
> Configuration both wants to serve the value with the new key.*
> *If the mapping is not specified before any retrieval happened, the value is
> only stored under the deprecated key but not the new key.*
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
