Skip to content

Conversation

@pkumar-singh
Copy link
Member

Motivation

Bookie was previously a concrete class that was used and abused all
over the place, especially in tests. A classic example of the God
object antipattern. The extensive use in tests, resulted in test cases
which spin up many instances of the whole system, which is very heavy
and very slow, especially when trying to unit tests a particular
feature.

This change is the first step to resolving this situation. Bookie is
now an interface, implemented by BookieImpl. Subsequent changes will
break out parts of the interface, cleanup calls and add dependency
injection.

@pkumar-singh pkumar-singh marked this pull request as draft May 22, 2021 00:16
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

very good.

I know that this is a "draft", but I left a couple of comments

thanks for this contribution

Bookie was previously a concrete class that was used and abused all
over the place, especially in tests. A classic example of the God
object antipattern. The extensive use in tests, resulted in test cases
which spin up many instances of the whole system, which is very heavy
and very slow, especially when trying to unit tests a particular
feature.

This change is the first step to resolving this situation. Bookie is
now an interface, implemented by BookieImpl. Subsequent changes will
break out parts of the interface, cleanup calls and add dependency
injection.
@pkumar-singh pkumar-singh marked this pull request as ready for review May 25, 2021 00:52
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Looks great

+1

Copy link
Contributor

@hsaputra hsaputra left a comment

Choose a reason for hiding this comment

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

+1
LGTM

@hsaputra
Copy link
Contributor

Will merge this by EOD if no more comment or concern. Thank you

@merlimat merlimat added this to the 4.15.0 milestone May 25, 2021
Copy link
Contributor

@dlg99 dlg99 left a comment

Choose a reason for hiding this comment

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

+1

@hsaputra hsaputra merged commit c1847af into apache:master May 25, 2021
Sunny-Island pushed a commit to Sunny-Island/bookkeeper that referenced this pull request Jun 2, 2021
### Motivation

Bookie was previously a concrete class that was used and abused all
over the place, especially in tests. A classic example of the God
object antipattern. The extensive use in tests, resulted in test cases
which spin up many instances of the whole system, which is very heavy
and very slow, especially when trying to unit tests a particular
feature.

This change is the first step to resolving this situation. Bookie is
now an interface, implemented by BookieImpl. Subsequent changes will
break out parts of the interface, cleanup calls and add dependency
injection.

Reviewers: Matteo Merli <mmerli@apache.org>, Andrey Yegorov, Henry Saputra <hsaputra@apache.org>, Enrico Olivelli <eolivelli@gmail.com>

This closes apache#2717 from pkumar-singh/merge_back_to_oss and squashes the following commits:

3edee49 [Prashant] Replace SettableFuture  with CompletableFuture
db69026 [Ivan Kelly] Turn Bookie into an interface
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
### Motivation

Bookie was previously a concrete class that was used and abused all
over the place, especially in tests. A classic example of the God
object antipattern. The extensive use in tests, resulted in test cases
which spin up many instances of the whole system, which is very heavy
and very slow, especially when trying to unit tests a particular
feature.

This change is the first step to resolving this situation. Bookie is
now an interface, implemented by BookieImpl. Subsequent changes will
break out parts of the interface, cleanup calls and add dependency
injection.

Reviewers: Matteo Merli <mmerli@apache.org>, Andrey Yegorov, Henry Saputra <hsaputra@apache.org>, Enrico Olivelli <eolivelli@gmail.com>

This closes apache#2717 from pkumar-singh/merge_back_to_oss and squashes the following commits:

3edee49 [Prashant] Replace SettableFuture  with CompletableFuture
db69026 [Ivan Kelly] Turn Bookie into an interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants