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

ACCUMULO-4597 fixed bug in rfile-info #229

Merged
merged 1 commit into from Mar 7, 2017

Conversation

Projects
None yet
5 participants
@keith-turner
Contributor

keith-turner commented Mar 6, 2017

No description provided.

@asfgit asfgit merged commit de80cf5 into apache:1.7 Mar 7, 2017

1 of 2 checks passed

Jenkins Looks like there's a problem with this pull request
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@phrocker

Looks good. I just made a comment about something that may or may not help provide additional debug information. I still give it a +1 as it was more of a suggestion for debugging than a requirement, in my opinion.

for (Entry<ByteSequence,MutableLong> entry : lcg.columnFamilies.entrySet()) {
setCF.add(entry.getKey());
if (lcg.columnFamilies == null) {
Preconditions.checkState(lcg.isDefaultLG, " Group %s has null families. Only expect default locality group to have null families.", lcg.name);

This comment has been minimized.

@phrocker

phrocker Mar 7, 2017

Contributor

Is there any other useful information that could be provided? For example, would it make sense to print out the rest of the locality group column families and help identify if others are listed as a the default and have no column families?

This comment has been minimized.

@phrocker

phrocker Mar 7, 2017

Contributor

haha and it was merged! It might be useful to take this as an opportunity to provide more information for debugging.

This comment has been minimized.

@keith-turner

keith-turner Mar 7, 2017

Contributor

You can thank findbugs for the message. I had some different code that accomplished the same thing and made findbugs unhappy. So I changed it to this to soothe findbugs. I don't expect this case to happen, and if it does I would have been happy with an NPE. There are many places that could have an NPE when there is a bug, we don't handle them all with detailed error messages.

Findbugs has found some nasty bugs. The false positives can be a little annoying, but I think its worth it.

This comment has been minimized.

@ctubbsii

ctubbsii Mar 8, 2017

Member

Findbugs has found some nasty bugs. The false positives can be a little annoying, but I think its worth it.

+1000

This comment has been minimized.

@phrocker

phrocker Mar 8, 2017

Contributor

"There are many places that could have an NPE when there is a bug, we don't handle them all with detailed error messages."
https://s-media-cache-ak0.pinimg.com/736x/88/15/5e/88155efa961948d0627e61236c7d1f2b.jpg

This comment has been minimized.

@keith-turner

keith-turner Mar 8, 2017

Contributor

@phrocker did you clear that meme with @mjwall

This comment has been minimized.

@mjwall

@keith-turner keith-turner deleted the keith-turner:ACCUMULO-4597 branch Jun 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment