Skip to content

Conversation

@rafaelweingartner
Copy link
Member

Fix new find bug error that was introduced in PR #1361
Report: http://jenkins.buildacloud.org/job/build-master-slowbuild/3455/findbugsResult/new/

It is the same fix as the one proposed in #1427; the difference is that this PR tried to change only the code that was strictly needed. However, I took the liberty to remove a dead code and few lines of code (annotations) that were not needed

S3Utils.getFile(s3, s3.getBucketName(), srcData.getPath(), destFile).waitForCompletion();


if (destFile == null) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this because there is no way this "destFile" variable can be null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this because new File(...) will always throw an exception if the value would be null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed that because the new File (...) will always create an object that is referenced by destFile; therefore, it will never be == null.

@rajap9711
Copy link

@swill @DaanHoogland tested this PR - LGTM.
All BVTs have passed with the exception of the following tests

  • test_07_list_default_iso - iso didn't exist.
  • test_01_test_vm_volume_snapshot - test case issue - operation is not allowed?

Raja

@swill
Copy link
Contributor

swill commented Mar 25, 2016

I don't think these tests failing are relevant to this change. We agree this is ready to be merged? Only against master?

@nvazquez
Copy link
Contributor

Code LGTM

@rafaelweingartner
Copy link
Member Author

Tests were performed and reviews executed; so, should we merge this or the #1427?

@swill
Copy link
Contributor

swill commented Mar 31, 2016

I think we should probably merge this one and not #1427. Do you agree?

@rafaelweingartner
Copy link
Member Author

I agree, but I was the one that opened the PR, so in my opinion, my opinion should no count much here :)

@DaanHoogland
Copy link
Contributor

I opened #1427 and I agree, @rafaelweingartner and @swill so please go ahead. I will close #1427

@asfgit asfgit merged commit b3de01a into apache:master Mar 31, 2016
asfgit pushed a commit that referenced this pull request Mar 31, 2016
Fix new error found in findbugs slow build #3455Fix new find bug error that was introduced in PR #1361
Report: http://jenkins.buildacloud.org/job/build-master-slowbuild/3455/findbugsResult/new/

It is the same fix as the one proposed in #1427; the difference is that this PR tried to change only the code that was strictly needed. However, I took the liberty to remove a dead code and few lines of code (annotations) that were not needed

* pr/1438:
  Fix findbugs slow build 3455

Signed-off-by: Rafael Weingärtner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants