-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
[logs] Dropping dt column #4587
[logs] Dropping dt column #4587
Conversation
b8a452a
to
acd146d
Compare
That's a breaking change though as Here's what the Airflow UPDATING.md look like: It can be simple at first, with just a list of version and related breaking changes and PR references. |
83e314b
to
255b252
Compare
@mistercrunch I added an |
7226dff
to
634791b
Compare
Codecov Report
@@ Coverage Diff @@
## master #4587 +/- ##
==========================================
+ Coverage 72.63% 72.64% +0.01%
==========================================
Files 205 205
Lines 15403 15395 -8
Branches 1183 1183
==========================================
- Hits 11188 11184 -4
+ Misses 4212 4208 -4
Partials 3 3
Continue to review full report at Codecov.
|
182d421
to
781e128
Compare
781e128
to
ad7556d
Compare
lgtm |
|
||
def upgrade(): | ||
with op.batch_alter_table('logs') as batch_op: | ||
batch_op.drop_column('dt') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-bodley let's avoid backward incompatible db migrations in the future as they require scheduling downtime to upgrade the app for people who use rolling deploys. This took our production down for a moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mistercrunch just to clarify what should have the logic been here? I'm simply asking to learn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, maybe we could have just left the column there as NULL moving forward and add a note about it being deprecated.
Whist looking through the Superset
logs
table I discovered that there was a misalignment of the date defined in thedt
column vsdttm
.The reason for the inconsistency is
dttm
is defined in reference to the UTC timezone whereasdt
use the date in reference to one's local timezone. One possible fix ishowever given that
dt
anddttm
aren't using the same now instance, there's still no guarantee there will be consistency between these columns especially around 12:00:00 am UTC.The fix here is simply to remove the
dt
column which is unnecessary as this information is completely defined by thedttm
column. Note I also removed the deprecatedactivity_per_day
which used theLog.dt
column which is no longer used in the code.Additionally tested by running:
to: @graceguo-supercat @michellethomas @mistercrunch @timifasubaa