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

Changed release behavior #6

Closed
wants to merge 4 commits into from
Closed

Changed release behavior #6

wants to merge 4 commits into from

Conversation

wuhkuh
Copy link

@wuhkuh wuhkuh commented Mar 2, 2017

  • Edited released to release (as it is a function)
  • More importantly, drain the queue when it is filled!

Caused issues in aedes, see moscajs/aedes#88

- Edited released to release (as it is a function)
- More importantly, drain the queue when it is filled!

Caused issues in aedes, see moscajs/aedes#88
@wuhkuh
Copy link
Author

wuhkuh commented Mar 2, 2017

There seem to be issues. Let me check what's wrong. (Thanks CI 👍)
What's strange is that I am actually getting the expected behavior, while the tests fail.

@mcollina
Copy link
Owner

mcollina commented Mar 2, 2017

Can you add also a unit test?

@wuhkuh
Copy link
Author

wuhkuh commented Mar 2, 2017

Okay, I finally see why released is released - it is integrated in fastparallel. I'm reverting the name changes and the tests are back up again. I'll start working on a unit test.

Remove one from the queue first, before adding one.
Changed fastparallel released property to work with the release function. Confusing names, huh?

TODO: fix unit test.
It was right the first time. Reverting this.
I'm adding a unit test to prevent this from happening again.
I removed the old 'queue concurrency' and replaced it with this one. Please review this change!

These unit tests functionalities are added:
- Check whether queue order is OK
- Check whether queue is emptied correctly
@wuhkuh
Copy link
Author

wuhkuh commented Mar 2, 2017

This took somewhat longer than expected. Could you review the change on the 'queue concurrency' unit test? I replaced it because it failed, and the test seemed erroneous because it tested asynchronously while the problems were synchronous.

I'm not 100% sure it should have failed to begin with. IMO the new tests should do this correctly.

@mcollina
Copy link
Owner

mcollina commented Mar 3, 2017

The overall reason why we have a concurrency is to limit the number of messages that are in flight. This happens to keep memory usage low and throughput high. It's a good thing.

If the queue is not being drained correctly it's a probably a bug in some Aedes code.
I have added a test for the overlapping subscriptions, and it seems working correctly.

@mcollina
Copy link
Owner

mcollina commented Mar 3, 2017

cc @GavinDmello

@wuhkuh
Copy link
Author

wuhkuh commented Mar 3, 2017

Oh, wow, you are right. This limits the queue to 150 instead of limiting the concurrency.
I'll check out the issue in aedes when I have time.

@wuhkuh wuhkuh closed this Mar 3, 2017
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.

2 participants