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

fix(logs): increase json field for logs table #24911

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

cnabro
Copy link
Contributor

@cnabro cnabro commented Aug 8, 2023

SUMMARY

  • Increase json field for logs table. (Text to MediumText)
  • When insert logs while imports a specific dashboard, An error occured.
(MySQLdb._exceptions.DataError) (1406, "Data too long for column 'json' at row 1")
[SQL: INSERT INTO logs (`action`, user_id, dashboard_id, slice_id, json, dttm, duration_ms, referrer) VALUES (%s, %s, %s, %s, %s, %s, %s, %s)]
[parameters: ('copy_dash', '1', 9, 0, '{"path": "/superset/copy_dash/9/", "data": "{\\"certified_by\\":\\"\\",\\"certification_details\\":\\"\\",\\"css\\":\\"\\",\\"dashboard_title\\":\\"G ... (171666 characters truncated) ... BS\\"}},\\"filter_scopes\\":\\"{}\\"}", "url_rule": "/superset/copy_dash/<int:dashboard_id>/", "object_ref": "Superset.copy_dash", "dashboard_id": 9}', datetime.datetime(2023, 8, 7, 6, 50, 6, 897685), 6474, '[http://__/superset/dashboard/9/?native_filters_key=A27QtFLKT95Qdd0X-h4KkTLZTBhOslCg0Jshcisx4A8FM71hI_Ae6pBdTc4ac94f')](http://__/superset/dashboard/9/?native_filters_key=A27QtFLKT95Qdd0X-h4KkTLZTBhOslCg0Jshcisx4A8FM71hI_Ae6pBdTc4ac94f%27))]

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

0769ef90fddd -> 2e826adca42c (head), Fix schema for log
ee179a490af9 -> 0769ef90fddd, Fix schema perm for datasets
e0f6f91c2055 -> ee179a490af9, deckgl-path-width-units
bf646a0c1501 -> e0f6f91c2055, create_user_favorite_table
a23c6f8b1280 -> bf646a0c1501, json_metadata

TESTING INSTRUCTIONS

ALTER TABLE logs MODIFY json MEDIUMTEXT;
Query OK, 556821 rows affected (5.10 sec)
  • Estimate 100000 rows updated per 1 sec

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@cnabro cnabro requested a review from a team as a code owner August 8, 2023 05:27
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #24911 (47a0eb0) into master (85a7d5c) will not change coverage.
The diff coverage is n/a.

❗ Current head 47a0eb0 differs from pull request most recent head fd3d4e5. Consider uploading reports for the commit fd3d4e5 to get more accurate results

@@           Coverage Diff           @@
##           master   #24911   +/-   ##
=======================================
  Coverage   69.00%   69.00%           
=======================================
  Files        1906     1906           
  Lines       74134    74134           
  Branches     8208     8208           
=======================================
  Hits        51153    51153           
  Misses      20858    20858           
  Partials     2123     2123           
Flag Coverage Δ
hive 54.17% <ø> (ø)
mysql 79.21% <ø> (ø)
postgres 79.31% <ø> (ø)
presto 54.07% <ø> (ø)
python 83.37% <ø> (ø)
sqlite 77.89% <ø> (ø)
unit 55.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

LGTM

@craig-rueda craig-rueda merged commit eb7c145 into apache:master Aug 8, 2023
29 checks passed
@john-bodley
Copy link
Member

john-bodley commented Aug 8, 2023

@cnabro thanks for the change. Would you mind also authoring a follow up PR which adds a line record to UPDATING.md which states that downtime is likely required in order to run this migration? A DDL operation (which locks the table) on the heavily used logs table will likely result in the service being inoperable.

Additionally I saw you checked the,

Runtime estimates and downtime expectations provided

checkbox, but I couldn't see any stats reported. Would you mind updating the PR description to include the necessary statistics.

@john-bodley john-bodley added the review:checkpoint Last PR reviewed during the daily review standup label Aug 8, 2023
@cnabro
Copy link
Contributor Author

cnabro commented Aug 9, 2023

@john-bodley Thank you. I'd added some notices in this PR (#24923).
And added additional descriptions for estimated times for this migration.

@john-bodley john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Aug 9, 2023
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 10, 2023
michael-s-molina pushed a commit that referenced this pull request Aug 10, 2023
@john-bodley
Copy link
Member

@cnabro looking over your PR I noticed that you didn't actually update the SQLAlchemy model definition. Would you mind authoring a follow up PR to include the model change?

@cnabro
Copy link
Contributor Author

cnabro commented Sep 28, 2023

@john-bodley Ok. I'll give you another PR 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants