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

Feat: Upgrade clickhouse server docker image #38

Merged
merged 3 commits into from
May 6, 2024

Conversation

FahadKhalid210
Copy link
Contributor

@FahadKhalid210 FahadKhalid210 commented Apr 29, 2024

fix #30
Upgrade clickhouse docker image and fix query issues due to deprecation of Live views.

We currently have 2 live views course_enrollments & course_block_completion. After upgrading clickhouse version, we encountered errors due to deprecation of live views.

image

After transitioning from live views to normal views to resolve the issue, we encountered a new challenge: course_enrollment & course_block_completion tables are only accessible in main db(openedx) and we get error on staff specified db’s. It's because staff user's don't have access to the tables that view statement is utilizing(_openedx_course_enrollments, _openedx_user_profiles & _openedx_users etc).

To resolve this, we've extended access to these tables for staff users and updated the corresponding row policy. This decision is backed by the fact that the live view already displays data from these tables through join operations.

RENAME TABLE _openedx_user_profiles TO openedx_user_profiles;
RENAME TABLE _openedx_users TO openedx_users;

DROP TABLE course_enrollments;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to drop the table here? Shouldn't it be view (considering view is being created right after that)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Table is already created using Live View command, that's why we need to drop it.


# The clickhouse repo is currently unavailable in some parts of the world. If we don't
# remove this repo here then `apt update` will fail. Check if the problem is resolved with:
# curl https://repo.yandex.ru/clickhouse/deb/stable/
# The above command should be a 200, and not a 404.
RUN rm /etc/apt/sources.list.d/clickhouse.list
# RUN rm /etc/apt/sources.list.d/clickhouse.list
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove this if it is not needed.

openedx_user_profiles.country AS user_country
FROM openedx_course_enrollments
INNER JOIN openedx_user_profiles ON openedx_course_enrollments.user_id = openedx_user_profiles.user_id
INNER JOIN openedx_users ON openedx_course_enrollments.user_id = openedx_users.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: end of file line missing

@FahadKhalid210 FahadKhalid210 merged commit 2b02ef8 into nightly May 6, 2024
@FahadKhalid210 FahadKhalid210 deleted the fahad-clickhouse#30 branch May 6, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants