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

feat(binary uuids): implement binary(16) uuids for cash table #152

Merged
merged 1 commit into from
Mar 23, 2016

Conversation

jniles
Copy link
Contributor

@jniles jniles commented Mar 1, 2016

This commit implements a proof-of-concept binary UUID storage scheme for
the cash module. The integration tests have been updated accordingly.

Overview
The introduction of binary uuids means that a little more work must be
done in the controllers and sql to insert/retrieve data. Some commands
in Node and SQL have been added to ease this process. I'll document
these differences as before and after scenarios.

READ

Before:

SELECT uuid FROM table;

After:

SELECT BUID(uuid) AS uuid FROM table;

Note: it is important to alias the column with AS in order to
preserve the property name uuid. The command BUID() has been added
to our MySQL database instance to convert binary uuids into hexadecimal
uuids.

WRITE

Before:

var insertSql = 'INSERT INTO table SET ?;';
var updateSql = 'UPDATE table SET ? WHERE uuid = ?;';

var data = {
  uuid : /** some uuid */,
  /* other properties ... */
};

db.exec(insertSql, [ data ]).then(/* ... */).catch(/* .. */);
db.exec(updateSql, [ data, req.params.uuid ]).then(/* ... */).catch(/* .. */);

After:

var insertSql = 'INSERT INTO table SET ?;';
var updateSql = 'UPDATE table SET ? WHERE uuid = ?;';

var data = {
  uuid : /** some uuid */,
  /* other properties ... */
};

// convert the hex uuid into a binary uuid
data.uuid = db.bid(data.uuid);

// insert with the new data
db.exec(insertSql, [ data ]).then(/* ... */).catch(/* .. */);
db.exec(updateSql, [ data, db.bid(req.params.uuid) ]).then(/* ... */).catch(/* .. */);

I'd be curious if we can come up with a better API for binary
conversions. The current method is pragmatic, but possibly unclear.

First implementation of #64.

@jniles
Copy link
Contributor Author

jniles commented Mar 1, 2016

@IMA-WorldHealth/local-contributors , what do you think of this method for binary UUIDs? If this is approved, we should quickly change ALL uuids to binary uuids, not just the cash table uuids.

@jniles jniles force-pushed the chore-binary-uuids branch 3 times, most recently from 0025836 to f8338c5 Compare March 2, 2016 14:06
'SELECT BUID(cash_uuid) AS uuid FROM cash_discard WHERE cash_uuid = ?;';

// parse the uuid into a buffer
const buffer = db.bid(id);
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 to have the same name as function, so that we should change the method db.bid(id) to db.buid(id).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One function takes HEX -- > BIN, the other takes BIN --> HEX. I was concerned that people might mix the two up if they had the same name.

If you feel strongly that they should be identically named, I can alter the API.

@DedrickEnc
Copy link
Contributor

@jniles
I seen the implementation and I am interested, so I want to know if you noticed any differency in terms of performance.
I am pretty sure that, we should get a good performance when fetching data SELECT query, but there is some additional steps for INSERT, UPDATE because we are not able to generate BUID directly.

@jniles
Copy link
Contributor Author

jniles commented Mar 4, 2016

@DedrickEnc , if there was a performance difference, I couldn't measure it. Since we never write more than a few rows at a time to the database, my guess is that the no user will ever feel a difference -- the difference would be fractions of a millisecond.

On read, I noticed slight increase in speed finding random binary UUIDs in a table of 25,000 rows -- but these differences were not statistically significant (too small sample size, too long to run tests).

@jniles jniles force-pushed the chore-binary-uuids branch 7 times, most recently from a62f3cf to cf224bf Compare March 8, 2016 08:13
@jniles
Copy link
Contributor Author

jniles commented Mar 9, 2016

@DedrickEnc, can I get a review?

@jniles jniles force-pushed the chore-binary-uuids branch 3 times, most recently from 07834e7 to 0790a1a Compare March 9, 2016 11:50
@jniles
Copy link
Contributor Author

jniles commented Mar 9, 2016

@DedrickEnc, is preferring let over var really going to block this PR? Or are there other problems with creating binary uuids?

@DedrickEnc
Copy link
Contributor

@jniles
It is more than that, you are about to introduce a new way of declaring variable and constant.
So it is not a small thing, a team must have a philosophy for coding.
The binary uuid is ok with me, just use var and I will merge the PR.
We will discuss about using var, let, and const when every one will be at kinshasa at april if that is okay with you.

@jniles
Copy link
Contributor Author

jniles commented Mar 9, 2016

Okay, we can discuss when everyone's back.

I will not change this because:

  1. It is valid JavaScript and supported in all our targeted versions.
  2. It is more precise to use a variable that is block-scoped. var is hoisted to the top of the function declaration, which is one of the strange design decisions in JavaScript. So, I've used both const for variables that are immutable and let for variables that are mutable. If it is of interest, here is a comparison of let versus var.
  3. @mbayopanda asked if we could use these new JavaScript features. We agreed as a team that we could use these. Now you have changed your mind, and it is unclear why. Just because it is new, does not mean it is bad.

I am deeply concerned in the number of pull requests that are blocked from comments not related to their:

  1. Features
  2. Performance
  3. Error handling
  4. Tests

These pull requests seem to be blocked for silly reasons, usually their choice of names or something similar. For this software to grow, we should learn how to prioritize the functionality over the code style. Particularly when we have not had discussions to say that one style is better than another.

I will reopen this pull request when we have a chance to discuss this.

@jniles jniles closed this Mar 9, 2016
@sfount
Copy link
Contributor

sfount commented Mar 23, 2016

As we are running out of time I am making the decision to open this pull request and I will evaluate the code. As we are using git to version control our codebase this can be undone if necessary once the team is together.

@sfount sfount reopened this Mar 23, 2016
This commit implements a proof-of-concept binary UUID storage scheme for
the cash module.  The integration tests have been updated accordingly.

**Overview**
The introduction of binary uuids means that a little more work must be
done in the controllers and sql to insert/retrieve data.  Some commands
in Node and SQL have been added to ease this process.  I'll document
these differences as `before` and `after` scenarios.

READ

Before:
```sql
SELECT uuid FROM table;
```

After:
```sql
SELECT BUID(uuid) AS uuid FROM table;
```
**Note**: it is important to alias the column with `AS` in order to
preserve the property name `uuid`.  The command `BUID()` has been added
to our MySQL database instance to convert binary uuids into hexadecimal
uuids.

WRITE

Before:
```js
var insertSql = 'INSERT INTO table SET ?;';
var updateSql = 'UPDATE table SET ? WHERE uuid = ?;';

var data = {
  uuid : /** some uuid */,
  /* other properties ... */
};

db.exec(insertSql, [ data ]).then(/* ... */).catch(/* .. */);
db.exec(updateSql, [ data, req.params.uuid ]).then(/* ... */).catch(/* .. */);
```

After:
```js
var insertSql = 'INSERT INTO table SET ?;';
var updateSql = 'UPDATE table SET ? WHERE uuid = ?;';

var data = {
  uuid : /** some uuid */,
  /* other properties ... */
};

// convert the hex uuid into a binary uuid
data.uuid = db.bid(data.uuid);

// insert with the new data
db.exec(insertSql, [ data ]).then(/* ... */).catch(/* .. */);
db.exec(updateSql, [ data, db.bid(req.params.uuid) ]).then(/* ... */).catch(/* .. */);
```

I'd be curious if we can come up with a better API for binary
conversions.  The currenct method is pragmatic, but possibly unclear.
@jniles
Copy link
Contributor Author

jniles commented Mar 23, 2016

@sfount, updated to latest master.

@@ -35,25 +35,29 @@ exports.debitNote = debitNote;

// looks up a single cash record and associated cash_items
// sets the "canceled" flag if a cash_discard record exists.
function lookupCashRecord(uuid, codes) {
function lookupCashRecord(id, codes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that this should be updated given #182. This is not directly related to the pull request and will not block it.

This should be addressed in the Cash 2.x Review.

@sfount
Copy link
Contributor

sfount commented Mar 23, 2016

To conclude the discussion in this thread before I close this; the team will discuss the use of newly introduced JavaScript features when they return from Tshikaji. Right now we are not in a position (both with geographical differences and time constraints) to have everything fully worked out before integration.

As everything in this pull request is supported in the controlled nodejs environment that we support I am confident that this code will work and pass all of the tests. If the team overrules this from a maintainability standpoint the code can be updated.

@sfount sfount merged commit 61bd9a4 into IMA-WorldHealth:master Mar 23, 2016
@jniles jniles deleted the chore-binary-uuids branch March 23, 2016 12:48
@jniles jniles mentioned this pull request Mar 24, 2016
sfount pushed a commit that referenced this pull request Jan 19, 2017
* feat(inventory): nicer inventory price list print

This commit improves the inventory item printed report by grouping by
inventory group and only showing part of the information as needed.

* fix(i18n): translate report title
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants