[CCI] BaseConnectionPool missing implementation of markAlive and markDead methods
#449
Conversation
Signed-off-by: Timur Bolotov <[email protected]>
Signed-off-by: Timur Bolotov <[email protected]>
Signed-off-by: Timur Bolotov <[email protected]>
timursaurus
requested review from
ananzh,
kavilla,
seanneumann,
dblock,
VachaShah,
nhtruong and
harshavamsi
as code owners
Signed-off-by: Timur Bolotov <[email protected]>
Signed-off-by: Timur Bolotov <[email protected]>
timursaurus
changed the title
BaseConnectionPool missing implementation of markAlive and markDead methodsBaseConnectionPool missing implementation of markAlive and markDead methods
| t.end(); | ||
| }); | ||
|
|
||
| t.test('call markDead twice', (t) => { |
There was a problem hiding this comment.
Why do you have to call markDead twice? Calling it once won't work?
There was a problem hiding this comment.
Why do you have to call markDead twice? Calling it once won't work?
#430 (review)
Calling it once does work, but I wanted to implement what @dblock suggested. I'm still not sure of it
| @@ -249,4 +249,28 @@ console.log('Deleting all PITs:'); | |||
| var response = await client.deleteAllPits(); | |||
|
|
|||
| console.log(response.body); | |||
| ``` | |||
|
|
|||
| ## Mark connection as `dead` | |||
There was a problem hiding this comment.
I'd explain what the purpose of this call is, and ensure it fails when called twice.
There was a problem hiding this comment.
By fail you mean a runtime error should be thrown when markDead is called on the same connection twice?
There was a problem hiding this comment.
I think so. Unless there's a reason not to.
I still don't quite get how and when this API is used?
There was a problem hiding this comment.
These methods are probably syntactical sugar for set status method (There only 2 valid connection Statuses: DEAD or ALIVE). I still don't understand why it must throw an error when you set status to DEAD twice though. Wouldn't that also introduce new business logic that will cause trouble for users who are marking a connection as DEAD again?
There was a problem hiding this comment.
Also since we have set status in Connection class, it makes more sense to move markDead and markAlive from ConnectionPool into Connection since you can change the status of the connection directly anyway. That is instead of client.connectionPool.markAlive(connection), we will do connection.markAlive(). A lot more convenient and less confusing because it has nothing to do with ConnectionPool.
There was a problem hiding this comment.
Sounds like an RFC to me
Should we implement this?
There was a problem hiding this comment.
I think we should move it to Connection for the reason mentioned above
|
@timursaurus do you wanna rebase this one so we can get it merged? Thank you :) |
Signed-off-by: Timur Bolotov <[email protected]>
Signed-off-by: Timur Bolotov <[email protected]>
Signed-off-by: Timur Bolotov <[email protected]>
|
Too stale |
Description
Implement
BaseConnectionPoolmarkDeadandmarkAlivemethodsFix unit test
Issues Resolved
Closes #429
Check List
yarn run lintdoesn't show any errorsBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.