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

Readd MockBookKeeper to Pulsar #2544

Merged
merged 2 commits into from
Sep 10, 2018
Merged

Conversation

ivankelly
Copy link
Contributor

These mocks were moved out of pulsar to the bookkeeper project a few
months ago. While it is good to have mocks generally available for
bookkeeper, if the mock is not in the pulsar code base and we want to
change the behaviour of the mock for a specific case, we need to wait
for a bookkeeper release cycle to do so. It's better to have the mock
in Pulsar, so we can bend it to our needs.

These mocks were moved out of pulsar to the bookkeeper project a few
months ago. While it is good to have mocks generally available for
bookkeeper, if the mock is not in the pulsar code base and we want to
change the behaviour of the mock for a specific case, we need to wait
for a bookkeeper release cycle to do so. It's better to have the mock
in Pulsar, so we can bend it to our needs.
@ivankelly ivankelly added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/test labels Sep 7, 2018
@ivankelly ivankelly self-assigned this Sep 7, 2018
@maskit
Copy link
Member

maskit commented Sep 7, 2018

Do the namespaces have to be "org.apache.bookkeeper.client"? These mocks are now Pulsar's, right?

(Am I the only one who read the subject as MacBookKeeper?)

@ivankelly
Copy link
Contributor Author

@maskit it does need to be in the same package because it's not a clean mock like we would have if just mocking an interface. It relies on some package private members of the classes it is overriding unfortunately. If we can move managed ledger to completely using the new bookkeeper apis, then the mocks can go in whatever package we want.

@ivankelly
Copy link
Contributor Author

rerun integration tests

Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

I see. In that case, the class name would be better to have some prefix such as "Pulsar" so it wouldn't conflict with other classes. Also, the prefix would make clear that this class is just for Pulsar and we can modify it freely.

@ivankelly
Copy link
Contributor Author

@maskit sure, will change it

Also changed how error injection works, to allow more control for the
caller. The caller can now ask for a promise that must be completed
before a the Nth request completes. If the promise completes with an
exception, that request is completed with the same exception.
@ivankelly
Copy link
Contributor Author

rerun java8 tests

@sijie sijie added this to the 2.2.0-incubating milestone Sep 10, 2018
@sijie sijie merged commit 69571f8 into apache:master Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants