Add ClickHouse event sink support #35
Conversation
itsjeyd
added
the
blocked by other work
|
@bmtcril Can you rebase this branch? |
Removing the charts, dashboards, datasets, and hard coded data for the coursegraph implementation we used for the demo. The new implementation is in the works and will have different table names, meaning all of these will need to be regenerated anyway.
This new package replaces the temporary coursegraph hack. This adds configuration for it to talk to our configured ClickHouse, handles table creation, and renames the coursegraph variables.
Type checking doesn't get us much in this repo since it's a lot of boilerplate.
openedx-event-sink-clickhouse has updated changes, so we need to change these columns and names to match.
This is just for testing. Once that branch merges we will release the package and update this PR to use the PyPI package here.
Also fix columns that were incorrectly nullable and fix up some primary key and sort columns.
bmtcril
force-pushed
the
bmtcril/add_event_sink
branch
from
28599fa to
aee62c3
Compare
|
I know the testing instructions are using a local env instead of a development one but either way, I tried with one, and it's failing when initializing: Is dev not supported yet? Update: this might be related to the missing table issue. |
|
@bmtcril I think you are missing adding the course_overview table schema in the clickhouse init job |
There was a problem hiding this comment.
@bmtcril This is working great, with two small exceptions:
-
If we're going to remove all the dashboards, we also need to update the superset_dashboard.py init script:
diff --git a/tutoroars/templates/oars/apps/pythonpath/superset-dashboard.py b/tutoroars/templates/oars/apps/pythonpath/superset-dashboard.py index c535202..b4e8925 100644 --- a/tutoroars/templates/oars/apps/pythonpath/superset-dashboard.py +++ b/tutoroars/templates/oars/apps/pythonpath/superset-dashboard.py @@ -61,11 +61,13 @@ def update_assets(): os.unlink(zip_file.name) # Mark the imported dashboard as Published - dashboard = superset.dashboards.find(slug=OPENEDX_DASHBOARD_SLUG)[0] - dashboard.published = True - # TODO: Enable feature flag DASHBOARD_RBAC, and set dashboard.roles = ["Open edX"] - # Consider finishing https://github.com/opus-42/superset-api-client/pull/31 - dashboard.save() + openedx_dashboards = superset.dashboards.find(slug=OPENEDX_DASHBOARD_SLUG) + if openedx_dashboards: + dashboard = openedx_dashboards[0] + dashboard.published = True + # TODO: Enable feature flag DASHBOARD_RBAC, and set dashboard.roles = ["Open edX"] + # Consider finishing https://github.com/opus-42/superset-api-client/pull/31 + dashboard.save()
-
I got errors about a missing
RALPH_HTTP_PROTOCOLvariable, so pulled in the changes from feat: add support for clickhouse cloud #38, and that fixed them.
👍
- I tested this with a fresh Tutor local devstack, and by following the PR instructions.
- I read through the code
-
I checked for accessibility issuesN/A -
Includes documentationN/A - Commit structure follows OEP-0051
Updates to make the table structure match the current implementation.
|
@mariajgrimaldi I just saw the issue with clickhouse connection that you saw on dev. It happened because I was trying to init before clickhouse was actually started. I tried it 2 secs later and it worked. I'm not sure how to solve this without putting a health check in, which we've seen requires a newer Docker Compose. 🤔 |
|
I think this is ready for final review if anyone else wants to take a look before tomorrow morning. |
…sion chore: pin event-routing-backends to 5.2.2
|
@bmtcril Is it ready for review? |
|
@Ian2012 @mariajgrimaldi yeah everything I know of is fixed |
|
@bmtcril Just one more thing, can you upgrade the event-routing-backends requirements to 5.3.0? |
This brings in some performance improvements and idempotent xAPI event ids!
|
I know that ERB 5.3.0 is messed up on Nutmeg, but I think in order to keep things moving I'll merge this and try to either fix that forward or roll back to the last release in another PR if that is what's needed. |
Replaces all "coursegraph" work with the new
event-sink-clickhouse. Mostly this is renaming things, but it does install theevent-sink-clickhouseplugin and configure it to talk to the Tutor-configured ClickHouse. Unfortunately this means that many downstream Superset assets needed to be deleted since they referenced the old names of things. New reports will be created as part of a separate set of tasks that Axim is contracting out.