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

Z3 timeout #855

Merged
merged 1 commit into from
Jan 18, 2018
Merged

Z3 timeout #855

merged 1 commit into from
Jan 18, 2018

Conversation

anothermattbrown
Copy link
Contributor

@anothermattbrown anothermattbrown commented Jan 16, 2018

Added a batfish setting for Z3's timeout parameter. Passed it to Z3 in Minesweeper's PropertyChecker and Batfish's NodJob and CompositNodJob classes. Had to add some plumbing to get the Settings object to PropertyChecker.


This change is Reviewable

@dhalperi
Copy link
Member

High-level LGTM.

Most of the code gets the settings by calling Main.getSettings() or something like that, but I actually prefer injecting it as your SMT changes are structured. Static shared state prevents us from running more than 1 worker in the same process, makes testing harder, etc.


Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


tests/basic/z3timeout.ref, line 11 at r1 (raw file):

        "   at org.batfish.main.Driver$1.run(Driver.java:367)",
        "Caused by: org.batfish.common.BatfishException: Error executing job",
        "   at org.batfish.job.BatfishJobExecutor.executeJobs(BatfishJobExecutor.java:129)",

This test is brittle -- it's going to require updating the ref anytime code changes (Batfish.java line numbers, for example).

I think there's a different test that simply expects an error, try to find that one?


tests/java-smt/commands, line 143 at r1 (raw file):


# z3 timeout - 1 second
add-batfish-option z3timeout 1000

why not 1ms?


Comments from Reviewable

@anothermattbrown
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


tests/basic/z3timeout.ref, line 11 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

This test is brittle -- it's going to require updating the ref anytime code changes (Batfish.java line numbers, for example).

I think there's a different test that simply expects an error, try to find that one?

Aha, looks like the ref file isn't checked for -error tests, so I could use an empty ref file instead.
The only other -error test is tests/basic/error.ref, which also could be made less brittle by emptying the file.


tests/java-smt/commands, line 143 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

why not 1ms?

good idea


Comments from Reviewable

Reasonably straightforward for questions based on NodJob and
CompositeNodJob, e.g. reachability.

Pretty messy for minesweeper questions based on PropertyChecker, since
it didn't have access to a Settings object. It would be nice to be able
to get at the Settings object from IBatfish, but the project layout
doesn't allow that (IBatfish is in batfish-common-protocol, Settings is
in batfish).
@dhalperi
Copy link
Member

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@dhalperi
Copy link
Member

:lgtm:


Comments from Reviewable

@dhalperi dhalperi merged commit 895284c into master Jan 18, 2018
@dhalperi dhalperi deleted the z3-timeout branch January 18, 2018 00:59
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

2 participants