refactor: move exit info related functions to smaller responsibility module #1503

boolafish · 2020-05-11T08:19:54Z

📋 ref https://github.com/omisego/new-tx-type-poc/issues/9

Overview

Move exit_info related DB access to PaymentExitInfo module instead. The motivation is to make the code easier to scale when we are adding new tx types. We will write new PaymentV2ExitInfo for example when we extend to payment v2.

First step to break the big DB module into smaller pieces!

Changes

Moving functions around from OMG.DB to OMG.DB.Models.PaymentExitInfo
expose get and get_all_by_type from OMG.DB for the db models module to call and simplify the code.
remove unused type param from decode_value and decode_values method of RocksDB.Core.

Question

Can we remove one of OMG.DBTest and OMG.RocksDBTest ? They are testing same thing now?

Testing

Move existing tests around
Add new tests for get/2, 3 and get_all_by_type/1, 2

coveralls · 2020-05-11T08:57:40Z

Coverage increased (+0.07%) to 78.211% when pulling 18583fc on boolafish/exit_info_db_with_tx_type_awareness into 5024988 on master.

boolafish · 2020-05-12T10:42:34Z

apps/omg_db/lib/db.ex

                      spent_blknum: 2,
                      block_hashes: 2,
-                      child_top_block_number: 1
+                      child_top_block_number: 1,
+                      get_single_value: 2


Did not add get/3 and get_all_by_type/2 as optional as it is used in the implementation of PaymentExitInfo

T-Dnzt

Approving with a comment on some logs not being moved to the new functions :)

apps/omg_db/lib/omg_db/rocks_db.ex

pnowosie

Wait, I didn't understand what we're trying to fix here...

apps/omg_db/lib/db.ex

apps/omg_db/lib/omg_db/models/payment_exit_info.ex

apps/omg_db/lib/omg_db/rocksdb/server.ex

apps/omg_db/lib/omg_db/models/payment_exit_info.ex

apps/omg_db/lib/db.ex

apps/omg_db/lib/omg_db/models/payment_exit_info.ex

boolafish · 2020-05-20T09:41:41Z

In a meeting/sync-up with @T-Dnzt and @pnowosie we decide to go with generic functions in DB/server exposing timeout as parameter for the model modules to fine-tune themselves. I will update the PR with that :)

boolafish · 2020-05-21T08:33:06Z

apps/omg_db/lib/omg_db/models/payment_exit_info.ex

+
+  def exit_infos(utxo_pos_list, server_name \\ @server_name)
+      when is_list(utxo_pos_list) do
+    DB.get(:exit_info, utxo_pos_list, @ten_seconds, server_name)


I am not 100% sure on the data size yet. 10 sec might be premature large?

Can you make it optional in DB.get with 5 seconds (default GenServer call's timeout)

okay. To introduce multiple optional args, I have to use opts \\ [] instead of timeout \\ 5000, server \\ @server_name to allow optional args combination in timeout and server. (unless there is better way of doing this?)

boolafish · 2020-05-21T08:35:50Z

apps/omg_db/lib/db.ex

+  This is generic DB function that can get the specific data of a specific type.
+  If it is a single value data, use get_single_value/1 instead.
+  """
+  def get(type, specific_keys, timeout), do: driver().get(type, specific_keys, timeout)


do we want to name this get_multi instead?

👍 yes, be specific!

rename the function to batch_get

pnowosie

Looks good! (just minors left)

pnowosie · 2020-05-21T12:29:12Z

apps/omg_db/lib/omg_db/models/payment_exit_info.ex

+
+  def exit_infos(utxo_pos_list, server_name \\ @server_name)
+      when is_list(utxo_pos_list) do
+    DB.get(:exit_info, utxo_pos_list, @ten_seconds, server_name)


Can you make it optional in DB.get with 5 seconds (default GenServer call's timeout)

pnowosie · 2020-05-21T12:32:15Z

apps/omg_db/lib/db.ex

+  This is generic DB function that can get the specific data of a specific type.
+  If it is a single value data, use get_single_value/1 instead.
+  """
+  def get(type, specific_keys, timeout), do: driver().get(type, specific_keys, timeout)


👍 yes, be specific!

So instead of calling the rock db implementation, it calls the DB interface. This removes the need of having the a PaymentExitInfo behaviour.

Receiving list of specific keys is more useful :) Also make changes to the PaymentExitInfo apis accordingly

Use keyword list to support multi optional args. (timeout and server)

boolafish force-pushed the boolafish/exit_info_db_with_tx_type_awareness branch from dce58bb to 2e681aa Compare May 11, 2020 08:39

boolafish force-pushed the boolafish/exit_info_db_with_tx_type_awareness branch 2 times, most recently from 5a8c49e to 2156ff2 Compare May 12, 2020 10:41

boolafish commented May 12, 2020

View reviewed changes

boolafish marked this pull request as ready for review May 12, 2020 10:42

boolafish requested review from pnowosie and T-Dnzt May 12, 2020 10:42

boolafish marked this pull request as draft May 12, 2020 10:49

boolafish marked this pull request as ready for review May 13, 2020 03:01

boolafish force-pushed the boolafish/exit_info_db_with_tx_type_awareness branch from 455cf87 to 9d23ad3 Compare May 13, 2020 03:01

boolafish requested a review from InoMurko May 13, 2020 03:02

boolafish force-pushed the boolafish/exit_info_db_with_tx_type_awareness branch from 9d23ad3 to e5aac6e Compare May 14, 2020 05:10

T-Dnzt approved these changes May 15, 2020

View reviewed changes

apps/omg_db/lib/omg_db/rocks_db.ex Show resolved Hide resolved

apps/omg_db/lib/omg_db/rocks_db.ex Show resolved Hide resolved

pnowosie reviewed May 18, 2020

View reviewed changes

apps/omg_db/lib/db.ex Show resolved Hide resolved

apps/omg_db/lib/db.ex Show resolved Hide resolved

apps/omg_db/lib/omg_db/models/payment_exit_info.ex Show resolved Hide resolved

apps/omg_db/lib/omg_db/rocksdb/server.ex Outdated Show resolved Hide resolved

boolafish force-pushed the boolafish/exit_info_db_with_tx_type_awareness branch 2 times, most recently from 9f58834 to 6180a5e Compare May 20, 2020 05:03

pnowosie suggested changes May 20, 2020

View reviewed changes

apps/omg_db/lib/omg_db/models/payment_exit_info.ex Show resolved Hide resolved

apps/omg_db/lib/db.ex Show resolved Hide resolved

apps/omg_db/lib/omg_db/models/payment_exit_info.ex Outdated Show resolved Hide resolved

boolafish commented May 21, 2020

View reviewed changes

boolafish requested review from pnowosie and T-Dnzt May 21, 2020 08:33

boolafish commented May 21, 2020

View reviewed changes

boolafish force-pushed the boolafish/exit_info_db_with_tx_type_awareness branch from 25996de to e0c44ea Compare May 21, 2020 10:10

pnowosie approved these changes May 21, 2020

View reviewed changes

boolafish added 5 commits May 22, 2020 15:21

feat: add get and get_all_by_type in rocksdb.server

eb910d8

feat: behaviour for PaymentExitInfo

23d6b87

feat: rocksdb impl of PaymentExitInfo for exit_infos

6a26ee6

feat: impl of in_flight_exits_info

c5cbba2

refactor: remove unused 'type' in decode_value(s)

e8b6ff7

boolafish added 15 commits May 22, 2020 15:21

feat: exit_info impl

fb49248

tests: PaymentExitInfo db test

ac09ce5

cleanup: remove exit info related codes from old DB modules and tests

9720280

fix: use PaymentExitInfo instead of DB

618a3d5

fix: behaviour module with driver

5f368a6

fix: make linter happy :)

7a46021

chore: add @impl flag for compile warning

819d5ea

chore: add get_single_value to @callback list

00984db

refactor: restrucure PaymentExitInfo to call OMG.DB directly

e8ba477

So instead of calling the rock db implementation, it calls the DB interface. This removes the need of having the a PaymentExitInfo behaviour.

test: unit test for get and get_all_by_type function

204a7f7

fix: default @server_name value in PaymentExitInfo

cf5fd4b

fix: rebase

841eb5f

feat: DB.get with list of specific keys instead

b61ddbf

Receiving list of specific keys is more useful :) Also make changes to the PaymentExitInfo apis accordingly

feat: generic DB function with timeout

9da9ede

refactor: rename function and add optional args for timeout

6593d83

Use keyword list to support multi optional args. (timeout and server)

boolafish force-pushed the boolafish/exit_info_db_with_tx_type_awareness branch from c761fc5 to 6593d83 Compare May 22, 2020 06:21

refactor: rename in_flight_exit_info() -> all_in_flight_exit_infos()

18583fc

T-Dnzt approved these changes May 22, 2020

View reviewed changes

boolafish merged commit dd5f33d into master May 22, 2020

boolafish deleted the boolafish/exit_info_db_with_tx_type_awareness branch May 22, 2020 08:49

unnawut added the chore Technical work that does not affect service behaviour label Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move exit info related functions to smaller responsibility module #1503

refactor: move exit info related functions to smaller responsibility module #1503

boolafish commented May 11, 2020 •

edited

Loading

coveralls commented May 11, 2020 •

edited

Loading

boolafish May 12, 2020

T-Dnzt left a comment

pnowosie left a comment •

edited

Loading

boolafish commented May 20, 2020 •

edited

Loading

boolafish May 21, 2020

pnowosie May 21, 2020

boolafish May 22, 2020

boolafish May 21, 2020

pnowosie May 21, 2020

boolafish May 22, 2020

pnowosie left a comment •

edited

Loading

pnowosie May 21, 2020

pnowosie May 21, 2020

refactor: move exit info related functions to smaller responsibility module #1503

refactor: move exit info related functions to smaller responsibility module #1503

Conversation

boolafish commented May 11, 2020 • edited Loading

Overview

Changes

Question

Testing

coveralls commented May 11, 2020 • edited Loading

Choose a reason for hiding this comment

T-Dnzt left a comment

Choose a reason for hiding this comment

pnowosie left a comment • edited Loading

Choose a reason for hiding this comment

boolafish commented May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnowosie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boolafish commented May 11, 2020 •

edited

Loading

coveralls commented May 11, 2020 •

edited

Loading

pnowosie left a comment •

edited

Loading

boolafish commented May 20, 2020 •

edited

Loading

pnowosie left a comment •

edited

Loading