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

[Batch Rewards] Schema & ETL Process for Sync #28

Merged
merged 5 commits into from
Feb 27, 2023

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Feb 15, 2023

Another part of #27 (based on #26)

This defined the "schema" for what will be uploaded to Dune. We reused/generalize some common code for the order rewards sync. More or less, this was a minor change. Still remaining todo for parsing the bytea[] type with participants field (since duneV2 uses lower case hex strings). We will want to replace all \x coming from postgres with 0x

@bh2smith bh2smith mentioned this pull request Feb 15, 2023
6 tasks
@bh2smith bh2smith requested review from harisang, sunce86 and a team February 15, 2023 15:14
@bh2smith bh2smith changed the title [Schema] add ETL Process for Batch Rewards Sync [Batch Rewards] Schema & ETL Process for Sync Feb 15, 2023
src/sync/orderbook.py Outdated Show resolved Hide resolved
src/sync/orderbook.py Outdated Show resolved Hide resolved
Copy link

@gentrexha gentrexha left a comment

Choose a reason for hiding this comment

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

Left one discussion comment and one minor question.

Other than that, LGTM!

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

How is this supposed to be run? As another pod running the same python script but with different arguments? Or is the same pod going to sync both order as well as batch rewards (we will need both for the transition period).

Some more comments/questions in line...

src/models/batch_rewards_schema.py Outdated Show resolved Hide resolved
src/models/batch_rewards_schema.py Outdated Show resolved Hide resolved
src/sql/orderbook/batch_rewards.sql Outdated Show resolved Hide resolved
src/sync/order_rewards.py Outdated Show resolved Hide resolved
src/fetch/orderbook.py Show resolved Hide resolved
@bh2smith
Copy link
Contributor Author

bh2smith commented Feb 21, 2023

How is this supposed to be run? As another pod running the same python script but with different arguments? Or is the same pod going to sync both order as well as batch rewards (we will need both for the transition period).

Initially this has been setup to run as another pod with different runtime arguments, but now that you mention it, we don't really need to do that since it will be phased out so soon. Perhaps we can just put both orderbook table syncs into the same job (they execute almost instantly and, use almost no computational resources and do nearly the same thing). Then later we can just drop the other job from here and version bump the production deployment!

Update: As cool as it might sound to run them both at the same time, it would require a bit of hacky refactoring (not relevant to this PR so I will not be doing that here).

@bh2smith bh2smith force-pushed the batch_rewards_query branch from 0ca61cc to 870aa21 Compare February 22, 2023 08:27
Base automatically changed from batch_rewards_query to main February 23, 2023 12:54
@bh2smith bh2smith force-pushed the batch_rewards_extract_transform branch from 709dc5c to 53d89bd Compare February 23, 2023 14:37
@bh2smith bh2smith requested a review from harisang February 23, 2023 14:49
@bh2smith
Copy link
Contributor Author

This PR is getting pretty old. All comments have been addressed and I plan to merge this today.

@bh2smith
Copy link
Contributor Author

Merging now as the general sentiment is correct. There have been some changes to the backend DB that will need to be addressed as a follow up once their structures have been deployed and populated.

@bh2smith bh2smith merged commit 752697c into main Feb 27, 2023
@bh2smith bh2smith deleted the batch_rewards_extract_transform branch February 27, 2023 09:19
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.

4 participants