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

Delete the temp directories created while forking #6358

Merged
merged 2 commits into from Oct 24, 2020
Merged

Conversation

ratulm
Copy link
Member

@ratulm ratulm commented Oct 24, 2020

No description provided.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #6358 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #6358   +/-   ##
=========================================
  Coverage     72.97%   72.97%           
- Complexity    35133    35134    +1     
=========================================
  Files          2829     2829           
  Lines        142698   142700    +2     
  Branches      17148    17148           
=========================================
+ Hits         104134   104140    +6     
+ Misses        30335    30332    -3     
+ Partials       8229     8228    -1     
Impacted Files Coverage Δ Complexity Δ
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.91% <100.00%> (+0.03%) 246.00 <0.00> (ø)
...src/main/java/org/batfish/coordinator/PoolMgr.java 59.52% <0.00%> (-1.20%) 15.00% <0.00%> (-1.00%)
...ain/java/org/batfish/storage/FileBasedStorage.java 86.19% <0.00%> (-0.28%) 249.00% <0.00%> (ø%)
...tfish/representation/cisco/CiscoConfiguration.java 86.02% <0.00%> (+0.13%) 493.00% <0.00%> (+1.00%)
...a/org/batfish/representation/aws/LoadBalancer.java 82.50% <0.00%> (+0.31%) 71.00% <0.00%> (+1.00%)
...col/src/main/java/org/batfish/role/InferRoles.java 90.90% <0.00%> (+1.36%) 51.00% <0.00%> (+1.00%)

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ratulm and @sfraint)


projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 1549 at r1 (raw file):

      // do not need this directory anymore
      CommonUtil.deleteDirectory(unzipDir);

FYI CommonUtil.createTempDirectory calls:

tempDir.toFile().deleteOnExit();

I hope the explicit deletion does not interfere.


projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 1598 at r1 (raw file):

      throw new BatfishException(String.format("Error forking snapshot: %s", e.getMessage()), e);
    } finally {
      CommonUtil.deleteDirectory(newSnapshotInputsDir);

Just use FileUtils.deleteDirectory in both places. This function already throws IOException. No new BatfishExceptions.

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ratulm and @sfraint)


projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 1596 at r1 (raw file):

          baseSnapshotId);
    } catch (Exception e) {
      throw new BatfishException(String.format("Error forking snapshot: %s", e.getMessage()), e);

We really need to clean up these exceptions.
FWICT e can be narrowed to BatfishException | UncheckedIOException.
Eventually we should get rid of BatfishException. I'd prefer you didn't throw a new one here, but you can defer fixing that until initSnapshot exceptions are improved.

Copy link
Member Author

@ratulm ratulm left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @arifogel and @sfraint)


projects/coordinator/src/main/java/org/batfish/coordinator/WorkMgr.java, line 1549 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

FYI CommonUtil.createTempDirectory calls:

tempDir.toFile().deleteOnExit();

I hope the explicit deletion does not interfere.

I don't think it does, and we are using this pattern in upload snapshot.

Copy link
Member

@arifogel arifogel left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @sfraint)

@ratulm ratulm merged commit 5aa7650 into master Oct 24, 2020
@ratulm ratulm deleted the delete-tmp-dir branch October 24, 2020 04:43
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.

None yet

3 participants