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

add unit test for more than 1 storm workers being launched in 1 mesos executor #81

Open
erikdw opened this issue Dec 10, 2015 · 0 comments
Labels

Comments

@erikdw
Copy link
Collaborator

erikdw commented Dec 10, 2015

We currently lack unit tests for the situation with "more than 1 storm workers being launched in 1 mesos executor". As a result we've made an ugly chain of changes related to supporting this behavior:

PR #65 created a subtle regression where we could never launch more than 1 worker in a given executor (so at most 1 worker process per topology per host).
PR #78 attempted to fix this issue, but also added a new bug, where we subtract executor resources from offers more than necessary.
PR #79 attempted to fix the bug introduced in PR #78, but it introduced another bug (a NullPointerException crash, where after the first pass through the computeResourcesForSlot() main loop, the executorCpuResources and executorMemResources variables are set to null and because subtractedExecutorResources was set to true on the 1st pass).
PR #82 then is attempting to fix the NPE introduced in PR #79.

So we should add a unit test that prevents these kinds of regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant