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

[CCI] BaseConnectionPool missing implementation of markAlive and markDead methods #449

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

- Added missing types for AwsSigv4SignerOptions.service ([#377](https://github.com/opensearch-project/opensearch-js/pull/377))
- Fixed deprecated folder mapping "./" in the "exports" field module resolution ([#416](https://github.com/opensearch-project/opensearch-js/pull/416))
- Fixed `BaseConnectionPool` markAlive and markDead and their tests ([#449](https://github.com/opensearch-project/opensearch-js/pull/449))

### Security

Expand Down
24 changes: 24 additions & 0 deletions USER_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -249,4 +249,28 @@ console.log('Deleting all PITs:');
var response = await client.deleteAllPits();

console.log(response.body);
```

## Mark connection as `dead`
Copy link
Member

Choose a reason for hiding this comment

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

I'd explain what the purpose of this call is, and ensure it fails when called twice.

Copy link
Collaborator Author

@timursaurus timursaurus Mar 23, 2023

Choose a reason for hiding this comment

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

By fail you mean a runtime error should be thrown when markDead is called on the same connection twice?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. Unless there's a reason not to.

I still don't quite get how and when this API is used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds like an RFC to me
Should we implement this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should move it to Connection for the reason mentioned above

```javascript
console.log('Marking connection as dead:');

var connection = client.connectionPool.getConnection()
// or var connection = client.connectionPool.connections[0]

if (connection !== null) {
client.connectionPool.markDead(connection)
}
```

## Mark connection as `alive`
```javascript
console.log('Marking connection as alive:');

var connection = client.connectionPool.getConnection()
// or var connection = client.connectionPool.connections[0]

if (connection !== null) {
client.connectionPool.markAlive(connection)
}
```
6 changes: 4 additions & 2 deletions lib/pool/BaseConnectionPool.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,13 @@ class BaseConnectionPool {
throw new Error('getConnection must be implemented');
}

markAlive() {
markAlive(connection) {
connection.status = Connection.statuses.ALIVE;
return this;
}

markDead() {
markDead(connection) {
connection.status = Connection.statuses.DEAD;
return this;
}

Expand Down
38 changes: 37 additions & 1 deletion test/unit/base-connection-pool.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,18 @@ test('API', (t) => {
let connection = pool.addConnection(href);
t.same(pool.markDead(connection), pool);
connection = pool.connections.find((c) => c.id === href);
t.equal(connection.status, Connection.statuses.ALIVE);
t.equal(connection.status, Connection.statuses.DEAD);
t.end();
});

t.test('call markDead twice', (t) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you have to call markDead twice? Calling it once won't work?

Copy link
Collaborator Author

@timursaurus timursaurus Mar 22, 2023

Choose a reason for hiding this comment

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

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

const pool = new BaseConnectionPool({ Connection, sniffEnabled: true });
const href = 'http://localhost:9200/';
let connection = pool.addConnection(href);
t.same(pool.markDead(connection), pool);
t.same(pool.markDead(connection), pool);
connection = pool.connections.find((c) => c.id === href);
t.equal(connection.status, Connection.statuses.DEAD);
t.end();
});

Expand All @@ -90,6 +101,31 @@ test('API', (t) => {
t.end();
});

t.test('call markAlive twice', (t) => {
const pool = new BaseConnectionPool({ Connection, sniffEnabled: true });
const href = 'http://localhost:9200/';
let connection = pool.addConnection(href);
t.same(pool.markAlive(connection), pool);
t.same(pool.markAlive(connection), pool);
connection = pool.connections.find((c) => c.id === href);
t.equal(connection.status, Connection.statuses.ALIVE);
t.end();
});

t.test('call markDead and markAlive', (t) => {
const pool = new BaseConnectionPool({ Connection, sniffEnabled: true });
const href = 'http://localhost:9200/';
let connection = pool.addConnection(href);
t.same(pool.markDead(connection), pool);
t.equal(connection.status, Connection.statuses.DEAD);
t.same(pool.markAlive(connection), pool);
t.equal(connection.status, Connection.statuses.ALIVE);
connection = pool.connections.find((c) => c.id === href);
t.equal(connection.status, Connection.statuses.ALIVE);
t.not(connection.status, Connection.statuses.DEAD);
t.end();
});

t.test('getConnection should throw', (t) => {
const pool = new BaseConnectionPool({ Connection });
const href = 'http://localhost:9200/';
Expand Down