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

Assignment-week4-databases-AliaTaher #53

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Alia-Taher
Copy link

No description provided.

@yunchen4 yunchen4 self-assigned this Nov 8, 2024
Copy link

@yunchen4 yunchen4 left a comment

Choose a reason for hiding this comment

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

Hello Alia,
Nice work! Most comments are suggestions. There's only one point that you need to rework, and it's a more advanced topic. If you have any questions, don't hesitate to ping me on slack.
Have a nice weekend!

Comment on lines +4 to +5
const DATABASE_NAME = "databaseWeek4";
const COLLECTION_NAME = "population";
Copy link

Choose a reason for hiding this comment

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

Nice 👍

Comment on lines +15 to +19
if (fromAccount.balance < amount) {
console.log("Insufficient balance transfer aborted.");
await session.abortTransaction();
return;
}
Copy link

Choose a reason for hiding this comment

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

Early returns and throwings are always encouraged!

const toBalance = toAccount.balance + amount;

const fromChange = {
change_number: fromAccount.account_changes.length + 1,
Copy link

Choose a reason for hiding this comment

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

suggestion: using fromAccount.account_changes.length + 1 to know the next change_number is OK.
However, in real life one account may have a lot more change records. We don't really care what those changes are, we just want to know the maximum change number. Fetching all changes with the account can be expensive (since you need to transfer more bytes in the network. When you download something big from Internet, it is always slower than dowloading smaller files), and we try to only fetch the columns we need.
One solution is to have a function to get only the maximum change number (you have learned MAX sql syntax), and when you fetch the account data you don't fetch the changes column.

};

const toChange = {
change_number: toAccount.account_changes.length + 1,
Copy link

Choose a reason for hiding this comment

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

Same change number suggestion applies here.

await collection.updateOne(
{ account_number: from },
{
$set: { balance: fromBalance },
Copy link

Choose a reason for hiding this comment

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

Needs work: if there's only one user updating the database at any certain time, using set won't cause any problem. However, if multiple users are trying to update the same data at the same time without knowing each other's action (concurrent), using set will cause big problem, because set will overwrite a value completely.
Let's assume there are three users: A, B and C. Both A and B are trying to send money to C, and they don't know each other. Currently C has 1000 in his balance. A and B start transaction at the same time.
In A's transaction, the current balance is 1000. A sends 500 to C, so fromBalance in A's transaction is 1500.
In B's transaction, the current balance is also 1000. B sends 300 to C, so fromBalance in B's transaction is 1300.
From C's point of view, his final balance should be 1800, right?
However, if A commits the transaction earlier, C's balance will first be 1500. Does B know this happened? NOOOOOO. When B starts the transaction he will have a snapshot of the start point, and this snapshot won't include changes from other transactions during B's transaction process. If the database system follows "last write win" principle, which is very common in real life, then after B commits his transaction, C's balance would be 1300, if set is used.
Could you try to find another way to convey the same change in the database but without this overwrite risk?

Copy link

@yunchen4 yunchen4 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants