Open
Conversation
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
serprex
force-pushed
the
bring-back-null-avro
branch
from
282364d to
90b57fe
Compare
despite our efforts, nulled values still frequently leak through
serprex
force-pushed
the
bring-back-null-avro
branch
from
90b57fe to
90b7203
Compare
ilidemi
reviewed
Oct 17, 2025
Contributor
ilidemi
left a comment
ilidemi
left a comment
There was a problem hiding this comment.
Logic LGTM. Please add test with and without the flag
ilidemi
added a commit
that referenced
this pull request
Nov 29, 2025
Current situation: 1. `failed to sync records: failed to write records to S3: failed to write records to OCF writer: failed to write record to OCF: some_column_100: avro: *avro.null is unsupported for Avro long` occurs once a month for one customer 2. The column in question is a nullable integer, all values in it are null, it is also inherited from a parent table 3. Parent table has same name but different schema 4. Parent and child attnums are different, so possibly column deletions were involved, and also child seems to have become inherited later on. 5. I was unable to reproduce the error with that information. It is possible the schema has changed since, so it would be nice to capture the exact data when we have it. This change is a spiritual successor of #3613, but defaults to strict behavior (which has been working fine for everyone else) and puts the lax one under a config. When lax is enabled, it collects all the inputs that go into deciding whether a column would be nullable under strict behavior, then some extra about table inheritance, and logs it later if any mismatch with strict was detected. Tested that the new logic runs and logs if the code is adjusted to under-do nullable, but as-is the test doesn't do much as the issue is not cleanly reproducible yet. Also adding a generic code notification metric that's easy to emit from anywhere, will set up a non-paging alert on it once this goes in. The plan is to enable the setting just for one service, leave it to bake for another month or two, then check back when the notification fires. After the issue is sorted out, all the null tracking can be removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
despite our efforts, nulled values still frequently leak through. put behind setting