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

retrieveLogsAndClear works in constant space #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

retrieveLogsAndClear works in constant space #2

wants to merge 2 commits into from

Conversation

tau3
Copy link

@tau3 tau3 commented Jan 16, 2018

No description provided.

@oluwasayo
Copy link
Contributor

Thanks for the contribution @tau3! There's a constraint by Cloudwatch that log events must be ordered sequentially by timestamp.

@tau3
Copy link
Author

tau3 commented Jan 19, 2018

oh, i thought the queue was naturally ordered. PR will improved then)

@oluwasayo
Copy link
Contributor

It's a concurrent queue being written to by many producers. Take for example an application server servicing multiple users concurrently on different worker threads.

@tau3
Copy link
Author

tau3 commented Jan 19, 2018

@oluwasayo please, check

@oluwasayo
Copy link
Contributor

The test still fails. You can run the test locally on your computer with mvn clean test (make sure you update your working copy with the latest base)

@tau3
Copy link
Author

tau3 commented Jan 20, 2018

i think there are some problems with unit test. junit does not guarantee execution order of unit tests, so relying on "append" test running before "retrieveLogsAndClear" is a mistake. depending on configuration, they can even be executed in separate VMs.

also making one test depend on results of another is a bad practice imo

@oluwasayo
Copy link
Contributor

You're absolutely right about test ordering (at least in most common cases), although that wasn't the reason the tests failed.

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