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 #800 avoid importing files for completed bulk transactions #837

Merged
merged 1 commit into from Jan 2, 2019

Conversation

@keith-turner
Copy link
Contributor

commented Dec 18, 2018

No description provided.

@keith-turner keith-turner requested a review from milleruntime Dec 18, 2018

private String prepareBulkImport(Master master, final VolumeManager fs, String dir,
String tableId) throws Exception {
final Path bulkDir = createNewBulkDir(fs, tableId);
@VisibleForTesting

This comment has been minimized.

Copy link
@ctubbsii

ctubbsii Dec 18, 2018

Member

I don't think it's really necessary to use this annotation, especially in classes that aren't public API. It will be obvious that it's needed for testing, because removing it will affect a test. An inline comment serves the same purpose, but without the dependency on guava.

This comment has been minimized.

Copy link
@keith-turner

keith-turner Dec 18, 2018

Author Contributor

I think I can inline this method.

I am neutral on using or not using @VisibleForTesting. If there is going to be a change I think it would be best to do it everywhere for consistency. To me the annotation means that code should only be called by test code and the declaring class. Theoretically deviations from this could be detected using static analysis. If such a static analysis tool exists it may be nice to leave it as an annotation.

This comment has been minimized.

Copy link
@keith-turner

keith-turner Dec 18, 2018

Author Contributor

Nevermind about inlining I was thinking of a change I made for #840 to use @VisibleForTesting. For this code I think I will leave it as is. I am ok with changing it everywhere in another PR.

@keith-turner

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

All ITs ran successfully for this change.

// expect this to fail
assignMapFiles(fateTxid, asCtx, extent, bulkLoadPath.toString(), status.getLen());
fail();
} catch (TApplicationException tae) {

This comment has been minimized.

Copy link
@milleruntime

milleruntime Dec 18, 2018

Contributor

Could use ExpectedException here.

This comment has been minimized.

Copy link
@keith-turner

keith-turner Dec 19, 2018

Author Contributor

I think that has to be the final step in the test because the exception is caught by junit and therefore jumps out of the function. I still want to do some verification after this so I don't think I can use that.

This comment has been minimized.

Copy link
@milleruntime

milleruntime Dec 20, 2018

Contributor

Interesting. I thought the ExpectedException worked just like a try/catch, but reported exceptions that didn't get thrown at the end.

fileRefMap.put(new FileRef(path.toString(), path), mapping.getValue());
}
@Override
public List<TKeyExtent> call() throws Exception {

This comment has been minimized.

Copy link
@milleruntime

milleruntime Dec 18, 2018

Contributor

Was moving the Arbitrator thread here the fix?

This comment has been minimized.

Copy link
@keith-turner

keith-turner Dec 19, 2018

Author Contributor

Yeap. Also I forgot to mention that ignoring whitespace makes this diff really small.

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

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@ctubbsii ctubbsii added v1.9.3 labels 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.