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.

Sign up for GitHub

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

Jump to bottom

feat(pooling): add an extension to provide pooling service #5681

Merged
merged 1 commit into from
Jul 14, 2020
Merged

feat(pooling): add an extension to provide pooling service #5681

merged 1 commit into from
Jul 14, 2020

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Jun 7, 2020 •
edited
Loading

https://github.com/strongloop/loopback-next/blob/binding-pool/extensions/pooling/README.md

billing.png

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@raymondfeng raymondfeng requested a review from bajtos as a code owner June 7, 2020 05:38
@raymondfeng raymondfeng self-assigned this Jun 7, 2020
@raymondfeng raymondfeng added Extensions IoC/Context @loopback/context: Dependency Injection, Inversion of Control labels Jun 7, 2020
@raymondfeng raymondfeng force-pushed the binding-pool branch 5 times, most recently from f600efe to f2ef1ac Compare June 8, 2020 04:34
@raymondfeng raymondfeng mentioned this pull request Jun 8, 2020
Open
@bajtos
Copy link
Member

bajtos commented Jun 9, 2020

+1 to implement this as an experimental extension.

I don't have bandwidth to review the proposal. Can you @raymondfeng please pick somebody from the team to become a co-owner of the new package and let that person review the initial version.

@raymondfeng raymondfeng requested a review from hacksparrow June 11, 2020 00:30
@raymondfeng raymondfeng force-pushed the binding-pool branch 2 times, most recently from f42a945 to 92f9fbd Compare June 12, 2020 04:58
@agnes512 agnes512 mentioned this pull request Jun 26, 2020
Closed
29 tasks
@raymondfeng raymondfeng force-pushed the binding-pool branch 2 times, most recently from 9fb50ec to c7e0470 Compare July 10, 2020 16:57
hacksparrow
hacksparrow reviewed Jul 14, 2020
View reviewed changes
extensions/pooling/src/__tests__/acceptance/pooling.acceptance.ts Outdated
});

it('acquires/releases a resource from the pool', async () => {
const poolService = await givenPoolService({max: 5});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not leave it out {max: 5}? It makes it look like the test has something to do with the parameter, when it is not. Same applies to other locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

hacksparrow
hacksparrow reviewed Jul 14, 2020
View reviewed changes
extensions/pooling/src/__tests__/acceptance/pooling.acceptance.ts

it('acquires/releases a resource from the pool', async () => {
const poolService = await givenPoolService({max: 5});
poolService.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

poolService.start(); is not called in some places. How does it work? This behavior should be documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

My assumption is that poolService will not be usable for anything (eg: poolService.run) unless it is started.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quoted from generic-pool:

autostart: boolean, should the pool start creating resources, initialize the evictor, etc once the constructor is called. If false, the pool can be started by calling pool.start, otherwise the first call to acquire will start the pool. (default true)

hacksparrow
hacksparrow reviewed Jul 14, 2020
View reviewed changes
extensions/pooling/src/pooling.ts
function invokePoolableMethod(resource: Poolable, method: keyof Poolable) {
if (typeof resource[method] === 'function') {
return resource[method]!();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

else: should we throw an error? Failing silently (eg: in case of a typo) doesn't sound good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really in this case as the hook method can be optional. I added more debug statements.

hacksparrow
hacksparrow reviewed Jul 14, 2020
View reviewed changes
extensions/pooling/src/pooling.ts
this.pool = createPool(factory, {
max: 8, // Default to 8 instances
...this.options.poolOptions,
autostart: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

If autostart is to be configurable, it must be specified before ...this.options.poolOptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally disable autostart to fit into our life cycles. Added more comments in the code.

hacksparrow
hacksparrow reviewed Jul 14, 2020
View reviewed changes
extensions/pooling/src/__tests__/unit/README.md
@@ -0,0 +1 @@
# Unit tests
Copy link
Contributor

Choose a reason for hiding this comment

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

How about some unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pooling service is a wrapper of generic-pool. I believe the extra functions have been covered by the acceptance tests.

@hacksparrow
Copy link
Contributor

Looks mostly good, left some comments.

@raymondfeng
Copy link
Contributor Author

@hacksparrow Thank you for the comments. PTAL.

@raymondfeng raymondfeng requested a review from hacksparrow July 14, 2020 15:49
hacksparrow
hacksparrow approved these changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@hacksparrow hacksparrow left a comment

Choose a reason for hiding this comment

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

LGTM

@raymondfeng raymondfeng merged commit 402e1e0 into master Jul 14, 2020
@raymondfeng raymondfeng deleted the binding-pool branch July 14, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hacksparrow hacksparrow hacksparrow approved these changes

@bajtos bajtos Awaiting requested review from bajtos

Assignees

@raymondfeng raymondfeng

Labels
Extensions IoC/Context @loopback/context: Dependency Injection, Inversion of Control
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raymondfeng @bajtos @hacksparrow