Async Connection Pool about nodejs-idb-pconnector HOT 41 CLOSED

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

Marking issue resolved since the dbPool.js has been merged. Nice work :)

from nodejs-idb-pconnector.

Good Point. I think that would be wise. I know both are busy with conferences this week.

I got the green light from @ThePrez to finalize and publish the pool to NPM.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse Sounds good. Should we have Jesse and Aaron take one more look too?

from nodejs-idb-pconnector.

@brianmjerome

Thanks , updated the PR with the minor changes. If everything looks ok I will merge the PR to the master branch.

From there maybe we can try to use the dbPool with some of our existing projects.

Finally , If all seems to be integrated correctly I will push a new version of idbp to npm which includes the dbpool.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse Looks good -- just some minor comments.

from nodejs-idb-pconnector.

@brianmjerome

Hey I updated the dbpool again with some cleanup of the code. Made the variable names more verbose for better readability.
Take a look at it again and shoot any suggestions.

Thanks

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse Much better so far!

There are little things that bug me like:

• There should a space (newline) after a variable declaration block
• Still some abbreviated stmt in the code.

Have a good weekend!

from nodejs-idb-pconnector.

@brianmjerome I revised the PR to reflect the comments on it.

https://bitbucket.org/litmis/nodejs-idb-pconnector/pull-requests/9/revised-dbpool-aggregates-example-added/diff

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse

All of the connections were not being retired.

I wasn't planning on having the retired connections removed, but removing them is fine. :-)

I put some initial comments in the Pull Request. I'll have more comments soon once I get to review it fully. Thanks for creating it!

from nodejs-idb-pconnector.

@brianmjerome

revised code: https://bitbucket.org/litmis/nodejs-idb-pconnector/pull-requests/9/revised-dbpool-aggregates-example-added/diff

Thanks for the suggestions. You brought up some valid points.

1. changed names to be more descriptive of the function actually does.

   • query() was changed to runSql();

   • pbe() was changed to prepareExecute();

   • pe() was removed now its functionality can be used by calling prepareExecute without the params [].

2. Skipped the simple input checks and allowed the try catch blocks to handle invalid input.

3. When binding params idb does define clob bind type but when or how would you use the CLOB in JS? Seems out of place.

4. Good catch with those finally blocks the return was occuring before it could run making it useless. I fixed that up per your revisions.

Should we care to return output parameters if available from the stored procedures? it is simple to do but not sure if we want to overly complicate these 'easy functiions'?

Some Comments:

After running some test cases I realized we could simplify the binding type of the parameters by always setting the type to inout.

Then the code does not have check each time & users do not have to pass an array of objects anymore. Pass an array of values to bind and thats it. The functionality remains the same.

Also noticed that the retireAll() function failed unit tests. All of the connections were not being retired. This was because in the loop retire() is called which removes

one connection from the pool and the indexes shift , but the iterator continues to iteraterate and skips the ones that got shifted down an index.

I used the old and true for (let i = 0 ...)loop and moved the index i back one after each await retire(). It works now.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse

Some thoughts:

• I'm not a fan of the function names -- there really is no benefit to the developer to make them so abbreviated. If one wants them shorter, then one could cache a variable with a reference to the function with a shorter name.
• I think we can assume the sql/params passed in are valid. If we are to validate sql on this end then we should probably use an sql-parser to truly validate it, not just check if it's a string. My vote is to let the developer implement that.
• I agree that pe is not really necessary. If there are no parameters to bind, then the query function should be used. (or in my snippet's case, executeSql)
• The parameters can also have a CLOB type so I made that the default instead of throwing an error.
• I don't think the finally should be used in these cases since the connection should be retired if there is an error. Also if it returns in the try would the code in the finally ever get executed?

Here are my suggestions in this snippet: https://bitbucket.org/snippets/bjerome/7ezpAb

from nodejs-idb-pconnector.

@brianmjerome @aaronbartell @ThePrez I pushed the dbPool to the repository and added a few aggregate functions to the pool. https://bitbucket.org/litmis/nodejs-idb-pconnector/src/master/lib/dbPool.js

Example using the aggregates: https://bitbucket.org/litmis/nodejs-idb-pconnector/src/master/examples/aggregates.js

1. query (sql) = like a quick execute function just takes in sql.
2. pe (sql) = prepares and executes and sql statement.
3. pbe(sql , params) = prepaes binds and executes sql statement.

The pe() and pbe() are very similar I was thinking we could just mkae this one function() if no params are passed just do prepare and execute()

Let me know what you guys think for improvements on the dbPool code.

from nodejs-idb-pconnector.

@brianmjerome its all good , Have a great weekend too!

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse Haha that's what I get for manually typing it in and rushing. Have a good weekend!

from nodejs-idb-pconnector.

@brianmjerome just cloned it and seen should add and extra ' ) ' to close out the if.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse Good catch. The index is just for debug, but I think if someone does an integer index it should be at least 0 (no negatives).

    // Indices can only be > 0 if integer-based
    if (typeof index === 'undefined' || (Number.isInteger(index) && index < 0)) {
      index = connections.length + 1;
    }

The v3 snippet has been updated to include the code above.

Edit: Can we get rid of that other console.log from the Connection class? Maybe when these classes get added the others can be cleaned up once more.

from nodejs-idb-pconnector.

@brianmjerome As far as the easy functions go I think we should "put the "easy" functions in the idb-pconnector module for less complexity". We could also include these functions within the ConnPool itself. The easier it is to access the better.

from nodejs-idb-pconnector.

@brianmjerome Hey I tried the pool out with the ibm i dash and it works. One thing I noticed is that when creating the connections two were created with index (id) of 1.

#!js
Creating Connection 1...
constructed, dbconn={}
Connection 1 created
Creating Connection 1...
constructed, dbconn={}
Connection 1 created
Creating Connection 2...
constructed, dbconn={}
Connection 2 created
Creating Connection 3...
constructed, dbconn={}
Connection 3 created
Creating Connection 4...
constructed, dbconn={}
Connection 4 created
Creating Connection 5...
constructed, dbconn={}
Connection 5 created
Creating Connection 6...
constructed, dbconn={}
Connection 6 created
Creating Connection 7...
constructed, dbconn={}
Connection 7 created

It was because during initialization its zero based so when first index 0 is passed to createConnection() if falls into the first if block and becomes connection 1. Then the next iteration 1 is the index skips the if block and connection one is created again.

#!js
 for (i = 0; i < me.incrementSize; i++) {
      me.createConnection(i);
    }
  }

#!js
createConnection(index) {
    let me = this,
      connections = me.connections;

    if (!index) {
      index = connections.length + 1;
    }

    me.log(`Creating Connection ${index}...`);
    me.connections.push(new DBConnection(index, {
      database: me.database,
      debug: me.debug
    }));
    me.log(`Connection ${index} created`);
  }

Simple fix would be to just say

#!js
   if (index === undefined) {
      index = connections.length + 1;
    }

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse I updated the v3 (there was a typo in the attach function:

https://bitbucket.org/snippets/bjerome/5ezGRk

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

If the DBPool is going to be included with the idb-pconnector module, then it'd be nice to have a new itoolkit (nodejs-itoolkit) module for Node 8 that imports the idb-pconnector and creates a few "easy" functions for the user. Or just put the "easy" functions in the idb-pconnector module for less complexity.

from nodejs-idb-pconnector.

@brianmjerome I will try some test cases for the DBPool and report back. After that I think we should include DBPool inside the idb-pconnector module.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse Haha no worries.. yeah the detach just re-instantiates the Statement and sets the connection available. They'll never be closed unless something goes wrong with it or the the process ends.

from nodejs-idb-pconnector.

@brianmjerome EDIT: Disregard About detach being called without the params. I See that what this will do is make the connection available again and clear and close statements on the DBConnection.

from nodejs-idb-pconnector.

@brianmjerome Thanks Man. the retire() function inside the pool looks great.

I noticed down in the detach method for the pool the connection.detach() was not being passed a param. Therefore the it will default with false and never close() the connection.

#!js
  /**
   * Frees a connection
   * @param connection
   */
  async detach(connection) {
    let me = this,
      index = connection.poolIndex;

    me.log(`Detaching Connection ${index}...`);
    await connection.detach();
    me.log(`Connection ${index} detached`);
  }

#!js
/**
   * Creates a new statement
   * @param retire If true, retires the connection so it can be removed from the pool
   * @returns True if retiring, or the detached connection
   */
  async detach(retire = false) {
    let me = this;

    try {
      if (retire) {
        await me.close();
      } else {
        await me.newStatement();
        me.setAvailable(true);
      }
    } catch (error) {
      return `Connection failed to ${retire ? 'retire' : 'detach'}. Connection will be retired until the process exits.`;
    }

As far as the aggregate functions go we could add those aggregates as methods to the Connection() Class in idbp. Since once a connection is created we will have access to getStatement() and then call the statement methods needed for the aggregates.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

Here is my dbPool v3: https://bitbucket.org/snippets/bjerome/5ezGRk

Usage involving retiring a connection:

#!javascript

const {DBPool} = require('dbPool');

let pool = new DBPool(),
  connection,
  result;

async function runSql(sql) {
  connection = pool.attach();

  try {
    result = await connection.getStatement().exec(sql);
    console.log(`Result: ${JSON.stringify(result)}`);
  } catch (error) {
    pool.retire(connection);
    return error;
  }
}

runSql('select ...');

I think the aggregated functions should be put in the idb-pconnector package.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

Glad to see all the comments! I will post a v3 tomorrow with modifications per these suggestions.

I can clean up detach as well for retiring connections to remove it from the connections completely. I threw the retire in this morning and didn't get a chance to evaluate it completely.

Edit: Removing elements from the connections array will get a little messy so I'll have to refactor how the pool loops through its connections.

Regardless if 2 managed arrays or one, both operations complexity to find an available connection would be O(N), or O(2N) = O(N), so it's really just preference I think.

I feel the "Aggregator" should be defined separately from the pool, but definitely a necessity.

from nodejs-idb-pconnector.

@brianmjerome I agree with @ThePrez We should also include aggregate functions within the conn pool to achieve commonly used tasks like exec() or prepare bind execute.

that way on the client side we can just call those aggregates pass the sql and it can call getStatement() & exec and just return the results (for the exec() example).

from nodejs-idb-pconnector.

@brianmjerome Ok that makes more sense about retiring. A code example showing how it used during errors would help out.

Question regarding retiring it looks like when retired the setAvailable attribute is set to false but the connection is still part of the pool.
Should we consider removing from the pool after retiring? This could reduce time spent looking for an available connection.

Also same question as @ThePrez why a new connection is created during detach?

@ThePrez @brianmjerome Where do we think is the best place to put the conn pool? I think it should be bundled within the idb-pconnector and exported as a class.

Thanks

from nodejs-idb-pconnector.

Original comment by Jesse G (Bitbucket: ThePrez, GitHub: ThePrez).

@brianmjerome, I like how your implementation contains the availability status within the connection itself. Makes the code simpler. On the other hand, managing the available and unavailable connections in separate arrays would make for better performance in cases where we have many in-use connections. For instance, if there are 500 connections and 497 are in use (probably by some application error), it would need to spin through a bunch to find a free connection. Whereas, with @abmusse's implementation, it would just grab from the ready list. Or is that just splitting performance hairs that don't need to be split?

Also, a question (for Brian). Why does the retire always fetch a new connection?

I like @aaronbartell's "easy" function (it's almost like Staples started writing Javascript). I had envisioned a separate class ("Aggregator" or somesuch) that would combine and simplify many operations, and would be instantiated with a connection pool. It does make sense to embed that in the pool itself.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse retire = true is used when there is a caught exception either on the connection or statement where it cannot be used/become available again so it is retired until the program execution ends. I am currently using it for my implementation of my runSql and runProgram functions that have try/catches using the connector. In a production environment I feel there wouldn't be many exceptions that lead to memory issues. Does this make sense? I can create a code scenario if needed.

from nodejs-idb-pconnector.

@brianmjerome Overall looks really good implements the functionality I used in my dbpool. Question on the detach method. What is meant by the notion of detach with the retire param , ie : detach(retire)

#!js

 async detach(retire) {
    let me = this;

    try {
      await me.close();
      me.newConnection();
      await me.newStatement();

      me.setAvailable(!retire);
    } catch (error) {
      return 'Connection failed to detach. Connection will be retired until the process exits.';
    }
  }

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

Anyone get to look at my version 2 or have more thoughts?

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

Here's my v2:

https://bitbucket.org/snippets/bjerome/re8z4g

Usage:

#!javascript

const {DBPool} = require('dbPool');

let pool = new DBPool(),
  connection,
  result;

async function execSql(sql) {
  connection = pool.attach();
  try {
    result = await connection.getStatement().exec(sql);
  } catch (error) {
    result = error;
  }
  console.log(`Result: ${JSON.stringify(result)}`);
}

execSql('select something as thing from table');

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse Of course :-) I'm working on a v2 of mine while I try to figure out the other issue #4.

from nodejs-idb-pconnector.

@brianmjerome Well Yeah you can add a property to the connection, maybe a boolean indicating whether its available.

As far as this implementation it was not meant to be production ready lol.

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse I'm not a fan of that implementation. The code can be a lot simpler. I don't want to sound like a jerk, though :-) Instead of managing multiple arrays of connections to determine availability, the connection itself should manage whether it's available. Also, instead of checking for 'undefined' values, you can use parameter defaults. Take a look at my implementation snippet to compare. In mine, I am only asserting that the connection will be established locally, so I would have to add some username/password checking if the connection is not LOCAL.

It looks like we all have a similar idea of an implementation, but we'll need some standardization and succinctness for a production-ready codebase. Also I believe no console.logs should be executed unless a debug mode is specified.

from nodejs-idb-pconnector.

For The Connection Pool here was My Implementation: @aaronbartell @brianmjerome

#!js

const idbp = require('idb-pconnector');

/**
 * @class - provide a connection pool class that can create
 * @constructor
*/
class ConnPool{
  constructor(dbname, user, passwd, SIZE_OF_POOL){
    this.available = [];
    this.unavailable = [];
    this.defaultSize = 3;
    //intialize the parameters that were passed in
    //check if a dbname was provided if not default to *LOCAL
    dbname !== undefined ? this.dbname = dbname : this.dbname = '*LOCAL';
    user !== undefined ? this.user = user : this.user = undefined;
    passwd !== undefined ? this.passwd = passwd : this.user = undefined;

    let check = isValidSize( SIZE_OF_POOL );
    if (check){

      console.log(`${SIZE_OF_POOL} connections will be intialized to the Pool`);
    } else {
      this.SIZE_OF_POOL = this.defaultSize;
      console.log(`${this.SIZE_OF_POOL} connections will be intialized to the Pool`);
    }
    //Create and load up the Connections into the Available []
    this.initPool();
  }
  /**
   * Intialize the connection pool with connections
  */
  initPool(){
    //the username and password were defined , try to connect with the credentials.
    if (this.user && this.passwd) {
      console.log( 'Trying to connect as: '+ this.user);
      for (let i =0; i< this.SIZE_OF_POOL; i++){
        console.log('DB NAME: '+this.dbname);
        let dbconn = new idbp.Connection().connect(this.dbname, this.user, this.passwd);
        dbconn.home = this;
        dbconn.id = i+1;
        this.available.push(dbconn);
      }
    }

    //error providing username or password , try to connect without providing.
    else {
      console.log( 'Username or password not specificed using Defaults:' );
      for (let i = 0; i< this.SIZE_OF_POOL; i++){
        console.log('DB NAME: '+this.dbname);
        let dbconn = new idbp.Connection().connect(this.dbname);
        dbconn.id = i+1;
        this.available.push(dbconn);
      }

    }

  }
  /**
     *
     * @returns - connection that can be used by a statement.
    */
  getConn(){
    var conn = this;
    //function to automatically genereate a new connection , called when none are available
    var refill = function(){
      var dbconn = new idbp.Connection().connect(conn.dbname);
      //used the size of unavailable as id reference because when refill is called size of available ==0 id 1 will be resued
      dbconn.id = conn.unavailable.length+1;
      conn.available.push(dbconn);

    };

    var dispense = function() {
      var removed = conn.available.shift();
      conn.unavailable.push(removed);
      return removed;
    };

    if (0 !== this.available.length){
      let removed = dispense();
      return removed;

    }
    //if the avaliable array is empty refill it and return the connection
    refill();
    let removed = dispense();
    return removed;

  }
  /**
     * Add a connection back to the Connection Pool.
     * @param {object} Conn
     */
  relinquish(conn){
    if (conn instanceof idbp.Connection){
      //copy over the connection to the end of the available []
      this.available.push(conn);
      //get the index of released connection from the unavailable []
      let index = this.unavailable.indexOf(conn);
      console.log('Index is: '+ index);
      //remove that connection from the unavilabe []
      //first param is the start postion , second param 1 says remove 1 item.In Plain english : remove one elment @ the value of 'index'.
      this.unavailable.splice(index, 1);
      return true;
    }
    //throw an error
    return false;

  }

  /**
     * close all the connections in the pool.
    */
  destroyPool(){
    this.available.forEach((conn) => {
      conn.close();
    });
    this.unavailable.forEach((conn) => {
      conn.close();
    });
  }


}
/**
 * helper function to determine if the requested size of the conn pool is valid.
 * @param {number} data
 */
function isValidSize(data){
  return typeof data === 'number' && data > 0 ? true : false;
}


// //testing
// let conn = new ConnPool(5);
// console.log( '\n----Origin----\n\tAvailable'+ JSON.stringify(conn.available) + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );
// let conn1 = conn.getConn();
// console.log('\nRemove1\n\tAvailable'+ JSON.stringify(conn.available) + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );
// let conn2 = conn.getConn();
// console.log('\nRemove2\n\tAvailable'+ JSON.stringify(conn.available) + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );
// let conn3 = conn.getConn();
// console.log('\nRemove3\n\tAvailable'+ JSON.stringify(conn.available) + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );
// //will create a connection on the fly and add it to the unavailable []
// let conn4 = conn.getConn();
// console.log('\nRemove4\n\tAvailable'+ JSON.stringify(conn.available) + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );

// //since removed now points to the 4th connection that one will be added back to the available []
// let check = conn.relinquish(conn4);
// console.log('\nLets See what Happened\n\tAvailable'+ JSON.stringify(conn.available) + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );

// console.log('\nAnotherOne\n\tAvailable'+ JSON.stringify(conn.available) + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );
// let removed = conn.getConn();
// console.log('\nAmotherOne\n\tAvailable'+ JSON.stringify(conn.available)
//             + '\n\tUnavailable' + JSON.stringify(conn.unavailable) );

//export the connection pool class
exports.ConnPool = ConnPool;

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

I think since this project already requires the 'idb-connector' (which already requires 'dba') we could write an async version for this project. I am refactoring mine over the weekend.

Edit:

Snippet: https://bitbucket.org/snippets/bjerome/zeRaoq

Usage:

#!javascript

const {DBPool} = require('../bin/dbPool'); //wherever the package is located.

let pool = new DBPool(),
  connection,
  result;

//async if using the pconnector Statement class in dbPool.js
async function fetchSql(sql) {
  connection = await pool.attach();
  result = await connection.stmt.exec(sql);
  console.log(`Result: ${JSON.stringify(result, null, 2)}`);
}

fetchSql('select something from table');

I think the fetch (or your easy function) should be separate from the connection pool class, but maybe the base package can include some of those that utilize the pool in this way for the user.

from nodejs-idb-pconnector.

Original comment by Aaron Bartell (Bitbucket: aaronbartell, GitHub: aaronbartell).

In my mind it should be included in the base repo; especially since there will be big issues without it.

Here's the one I put together: https://bitbucket.org/snippets/litmis/9ebGyr

from nodejs-idb-pconnector.

Original comment by Brian Jerome (Bitbucket: bjerome, GitHub: brianmjerome).

@abmusse I'm currently working on testing the pconnector and refactoring my project code to use async/await so as I rewrite my version of the connection pool I'll share it here.

from nodejs-idb-pconnector.

@brianmjerome I have also been working on a connection pool class mainly to be used for the idb-p connector but could allow it to work for either idb connector.

It should definitely be implemented and @ThePrez has strongly encouraged its need to me.

We can both share what we have so far and collaborate on the connection pool and go from there.

Thanks!

from nodejs-idb-pconnector.