Skip to content
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

fixes #819 Handle bad locality group config. #840

Merged
merged 4 commits into from Jan 2, 2019

Conversation

@keith-turner
Copy link
Contributor

commented Dec 18, 2018

Two changes were made for bad locality group config. First, when setting
properties a warning is issued if the change results in bad LG config.
Second, minor compactions will log a warning and proceed with no LG config
if the config is bad.

fixes #819 Handle bad locality group config.
Two changes were made for bad locality group config.  First, when setting
properties a warning  is issued if the change results in bad LG config.
Second, minor compactions will log a warning and proceed with no LG config
if the config is bad.

keith-turner added some commits Dec 18, 2018

@keith-turner keith-turner force-pushed the keith-turner:accumulo-819 branch from a32c245 to b6252f5 Dec 19, 2018

+ "' resulted in bad locality group config. This may be a transient situation since "
+ "the config spreads over multiple properties. Setting properties in a different "
+ "order may help. Even though this warning was displayed, the property was updated. "
+ "Please check your config to ensure consistency.", e);

This comment has been minimized.

Copy link
@milleruntime

milleruntime Dec 20, 2018

Contributor

Why update the property if it is going to result in a bad locality group? If it is transient, then I would think that would be less reason to update the property. If it is not transient, then I would think you'd want to keep warning until its fixed.

This comment has been minimized.

Copy link
@keith-turner

keith-turner Dec 20, 2018

Author Contributor

Why update the property if it is going to result in a bad locality group?

It's because locality group config is spread across multiple properties. So there could be existing user code like the following.

 setProp(k1,v1); //makes LG group config incomplete/bad
 setProp(k2,v2); //makes LG group config incomplete/bad
 setProp(k3,v3); //makes LG config complete/good

So the transient bad state is very short lived and the user may have never noticed any problems from it. I do not want to break existing code that is working for 1.9.3. However it would be good to bring this to the users attention.


import com.google.common.collect.Iterables;

public class BadLocalityGroupMincIT extends AccumuloClusterHarness {

This comment has been minimized.

Copy link
@milleruntime

milleruntime Dec 20, 2018

Contributor

Since we have some long running tests, it would be nice to have a javadoc that this test should not time out. Or something mentioning that the last delete should not hang.

This comment has been minimized.

Copy link
@keith-turner

keith-turner Dec 20, 2018

Author Contributor

Oh I should remove that comment because I don't expect delete table to hang. That comment came from me starting off by copying a test and I noticed it had that delete at the end so I left it.

all.addAll(entry.getValue());
}

for (Entry<String,Set<Text>> entry : groups.entrySet()) {
Set<Text> colFams = entry.getValue();
String value = LocalityGroupUtil.encodeColumnFamilies(colFams);
setProperty(tableName, Property.TABLE_LOCALITY_GROUP_PREFIX + entry.getKey(), value);
setPropertyNoChecks(tableName, Property.TABLE_LOCALITY_GROUP_PREFIX + entry.getKey(), value);

This comment has been minimized.

Copy link
@milleruntime

milleruntime Dec 20, 2018

Contributor

Wouldn't you also want to check here?

This comment has been minimized.

Copy link
@keith-turner

keith-turner Dec 20, 2018

Author Contributor

This code is setting properties in the correct order and I did not want it to read all properties for each property set by this code.

} catch (TableNotFoundException e) {
throw new AccumuloException(e);
}
}

private void removePropertyNoChecks(final String tableName, final String property)

This comment has been minimized.

Copy link
@milleruntime

milleruntime Dec 20, 2018

Contributor

Could rename this method to "removePropNoLocalityChecks". Or something to indicate locality groups.

@keith-turner keith-turner merged commit 3862a40 into apache:1.9 Jan 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@keith-turner keith-turner deleted the keith-turner:accumulo-819 branch Jan 2, 2019

@ctubbsii ctubbsii added this to Done in 1.9.3 Jun 14, 2019

@ctubbsii ctubbsii added this to Done in 2.0.0 Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.