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

Improved abstractions + better object construction #96

Merged
merged 12 commits into from
Jan 29, 2020

Conversation

truthbk
Copy link
Member

@truthbk truthbk commented Dec 30, 2019

Introduces breaking changes.

Better description pending...

Copy link
Contributor

@ogaca-dd ogaca-dd left a comment

Choose a reason for hiding this comment

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

LGTM, added minor comments.

Out of the scope of this PR:
StatsDBlockingProcessor.run and StatsDNonBlockingProcessor.run share common code that might be factorized.

import java.nio.charset.Charset;
import java.util.concurrent.Callable;

import java.util.Queue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Several imports are unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have to add linting to the CI - thank you for checking. I don't use an IDE, so..... 😊

}
}

boolean isShutdown() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: As shutdown is defined in StatsDProcessor, isShutdown() and shutdown() could be moved to StatsDProcessor. Same comment could also be applied for StatsDNonBlockingProcessor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, code was already defined in StatsdProcessor just forgot doing this cleanup. Thank you! Great catch again.

qSize.incrementAndGet();
return true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't expect an InterruptedException to happen there as we are dealing with a non-blocking queue in this case and the offer() method would just return as opposed to stay there waiting for the write.

}
} catch (final InterruptedException e) {
if (shutdown) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch, yup!

Copy link
Contributor

@ogaca-dd ogaca-dd left a comment

Choose a reason for hiding this comment

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

LGTM.

{ 30, 17255, 2048, 2, 2 }, // 30 seconds, 17255 port, 2048 qSize, 2 sender workers, 2 processor workers
{ 30, 17255, 4096, 2, 2 } // 30 seconds, 17255 port, 4096 qSize, 2 sender workers, 2 processor workers
// // { 30, 17255, Integer.MAX_VALUE, 2 } // 30 seconds, 17255 port, MAX_VALUE qSize, 2 sender workers
{ 30, false, 256, 1, 1 }, // 30 seconds, non-blocking, 256 qSize, 1 worker
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This collection could be created with 3 "for loop" (non-blocking, Qsize, worker count)

@truthbk
Copy link
Member Author

truthbk commented Jan 29, 2020

Addressed changes + fixed CI to skip checkstyle on Java7, and compile successfully.

Merging onto #95

@truthbk truthbk merged commit ca83be3 into jaime/buffer_pool Jan 29, 2020
@truthbk truthbk deleted the jaime/abstractions branch January 29, 2020 20:34
@truthbk truthbk added this to the 2.10.0 milestone May 5, 2020
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