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

docs: add rfc for storage architecture #1290

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

Conversation

kakcy
Copy link
Contributor

@kakcy kakcy commented Oct 21, 2024

The main goal is to remove the mysql dependency from the API layer, and we suggest a solution by modifying the storage layer architecture. #1252

@hvn2k1
Copy link
Contributor

hvn2k1 commented Oct 29, 2024

let say if we have an API that do update account, after success updating, we need to query the feature table to get number of features then return both value to client. Would you call account storage to update account then call feature storage or would you do it in one storage?

@kakcy
Copy link
Contributor Author

kakcy commented Oct 29, 2024

@hvn2k1
#1290 (comment)

Thank you for your comment.

Is this question about how to maintain consistency when multiple storages need to be updated in one request?

editor *eventproto.Editor,
email, organizationID string,
) error {
// No DB operations such as transaction processing are performed, just call Update.
Copy link
Collaborator

@Ubisoft-potato Ubisoft-potato Oct 29, 2024

Choose a reason for hiding this comment

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

I have the same question with @hvn2k1 #1290 (comment), some API will need to operate on more than one table, can you please provide more examples about how to do this transaction in API layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you everyone for your comments.

I'll certainly think of a way to do this and update the documentation, as I need to consider this case.

@hvn2k1
Copy link
Contributor

hvn2k1 commented Oct 29, 2024

@hvn2k1 #1290 (comment)

Thank you for your comment.

Is this question about how to maintain consistency when multiple storages need to be updated in one request?

@kakcy yes, sometimes when an API needs to be updated, the function's behavior in a storage needs to be changed to, but what if that function is used in another API? Maybe we can just update it without affecting the other, but what if it is too hard or the changes making the function's main purpose being vague?

In my old project, we also faced this question and we haven't fully answered it, but we combine both approach (use multiple storage or stay with only one depend on the situation). I may suggest this pattern:

  • The API layer is for receiving and transforming the request data object, or, in other words, parse request into data transfer object (dto).
  • The handler layer is to handle main business logic. For example: validate input, then call update account and call count features separately.
  • The repo layer (or storage layer) is to execute our db queries, the programmer can choose to use single or multiple storages if suitable.
  • If the requirement making the query too complex, implement another function in repo layer to handle this logic is ok.

api drawio

Note: personally, I don't think there is a correct answer to this kind of question, but we can choose what is appropriate for different situations and it depends on the programmer's flexibility to make the code readable, reusable and extendable.

@kakcy
Copy link
Contributor Author

kakcy commented Oct 31, 2024

I tried to think of a way to maintain consistency in updating multiple tables from the API side, as shown in the sequence diagram below.

  • By creating storage instances using the same storage client instance (e.g. mySQLClient, etc.) and controlling transactions using the same transaction instance between different storages, consistency can be maintained for rollbacks and commits.
  • On the API side, if all storage operations are successful, Commit must be called at the end.
_9___API層_MySQL依存解消

We welcome your comments and questions.

@kakcy
Copy link
Contributor Author

kakcy commented Nov 12, 2024

@hvn2k1 @Ubisoft-potato
Please check the comments below when you have free time and look forward to your comments.
#1290 (comment)

@hvn2k1
Copy link
Contributor

hvn2k1 commented Nov 12, 2024

#1290 (comment)
@kakcy sorry for late reply! I wonder why we only call commit() on countStorage?

@kakcy
Copy link
Contributor Author

kakcy commented Nov 13, 2024

@hvn2k1
I tried to think of a way to maintain consistency when updating multiple tables from the API side, as shown in the sequence diagram below.
*Unlike the previous proposal, there is no need to call commit() from the API side.

You can maintain rollback and commit consistency by using the same storage client instance (such as mySQLClient) to create storage instances and using the same transaction instance to control transactions between different storages.

_9___API層_MySQL依存解消

@Ubisoft-potato
Copy link
Collaborator

@kakcy The new transaction flow looks good to me! 👍
As @hvn2k1 said we don't need to call commit mannually, the current implemetation will commit
transaction automatically after all operation finished.

@hvn2k1
Copy link
Contributor

hvn2k1 commented Nov 13, 2024

ahh I see, it looks awesome! 💯

@kakcy
Copy link
Contributor Author

kakcy commented Nov 14, 2024

Thank you for your review!
I have updated the RFC regarding the transaction you pointed out.

@Ubisoft-potato
Copy link
Collaborator

@cre8ivejp @kakcy The current solution proposal looks good to me.
I think it can resolve the MySQL storage layer dependency architecture issue.

@hvn2k1
Copy link
Contributor

hvn2k1 commented Nov 25, 2024

@cre8ivejp @kakcy this solution LGTM! 🚀

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.

3 participants