Escape the metadata column of the SQL status store - #2076
Conversation
cd658ed to
52fa39f
Compare
dpol1
left a comment
dpol1
left a comment
There was a problem hiding this comment.
LGTM. Checked the legacy path against StringTabScheme on a pile of random rows, identical, so "no migration" holds.
Two one-liners, not blocking. The new catch around Instant.parse in AbstractStatusUpdaterBolt only takes DateTimeParseException, but Date.from throws IllegalArgumentException on overflow, so +1000000000-12-31T23:59:59Z still escapes execute(). And the loop in MetadataColumn.encode NPEs on an empty key: getValues("") returns null while keySet() lists it, a legacy \t=x row gets you there.
The SQL status store flattened the metadata into one column as tab separated key=value pairs without escaping, so a value containing a tabulation came back from SQLSpout as extra keys and any legitimate value with a tabulation was corrupted. Keys and values are now escaped when the column is written and unescaped when it is read, which makes the round trip lossless rather than dropping characters silently. Columns written by the previous code start with a tabulation, columns written now start with a format marker, so existing rows keep being decoded exactly as before. Rows written from now on are not readable by earlier versions, which see the marker as an extra key. SQLSpout no longer decodes the column with StringTabScheme, whose format cannot represent a tabulation in a value. AbstractStatusUpdaterBolt also parsed status.store.as.is.with.nextfetchdate outside its try/catch, so a stored value that is not a valid instant threw out of execute. It is now logged and the URL scheduled normally, including a value which parses as an instant but does not fit a java.util.Date. Encoding a metadata no longer fails on a key which holds no readable value, such as the empty key a column written before the escaping can produce.
52fa39f to
a9365f0
Compare
|
Both fixed: the catch is now |
3 participants
StatusUpdaterBoltwrote the metadata column by concatenating\t+ key +=+ value with no escaping, and the read side split on those same characters, so a value containing a tabulation came back as one or more extra keys. The column is now written in an escaped form behind av1marker; rows written by earlier versions are still decoded exactly as before, so no migration is needed. Note a downgrade is not clean, and existing rows are not repaired: worth grepping the metadata column of an existing table for tabulations.Also guards
Date.from(Instant.parse(...))inAbstractStatusUpdaterBolt, which sat outside the try/catch, so an unparsablestatus.store.as.is.with.nextfetchdateno longer takes the bolt down; it is logged and the URL goes through the normal scheduler.SQLSpoutno longer routes throughStringTabScheme, whose behaviour is unchanged for the seed-file spouts that rely on it.For all changes
Is there a issue associated with this PR? Is it referenced in the commit message? - no issue
Does your PR title start with
#XXXXwhereXXXXis the issue number you are trying to resolve? - no issueHas your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
Is the code properly formatted with
mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?For code changes
mvn clean verify? -coreand the fullexternal/sqlsuite against the MySQL container (18 tests, all green), not a fullmvn clean verify