spanconfigsqlwatcher: add a system.protected_ts_records decoder#74913
spanconfigsqlwatcher: add a system.protected_ts_records decoder#74913craig[bot] merged 1 commit intocockroachdb:masterfrom
system.protected_ts_records decoder#74913Conversation
|
This change is |
|
@dt adding you for the small change to |
c16f06c to
2793d85
Compare
I'll defer to @tbg or @RaduBerinde if we want the proto message to be comparable (I'd guess yes -- the go struct was already comparable -- so I don't see why not, but we treat TenantID with care so let's check) |
RaduBerinde
left a comment
There was a problem hiding this comment.
Same here, I don't see why not.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @dt)
|
That seems ok. |
| s0 := tc.Server(0) | ||
| ptp := s0.DistSQLServer().(*distsql.ServerImpl).ServerConfig.ProtectedTimestampProvider | ||
| jr := s0.JobRegistry().(*jobs.Registry) | ||
| k := keys.SystemSQLCodec.TablePrefix(keys.ProtectedTimestampsRecordsTableID) |
There was a problem hiding this comment.
Instead of writing to the real table in this unit test, it'd be better isolated if using a scratch table instead:
Given this test should only care about the decoding routines, I would suggest writing to the scratch table in this way directly, instead of depending on jobs or the protected ts subsystem to write to the real table (and also needing to release the record). Also, since the column is nullable, worth having a nil check (an empty proto should be returned -- something we'll need to explicitly handle).
There was a problem hiding this comment.
Hmm doesn't it make sense to test this with the jobsprotectedts.MakeRecord, Protect, and Release methods that are actually going to be used to interact with this table by all PTS consumers? We use this pattern in many other PTS tests as well. In theory, target is a free-form bytes column and the decoder+test could be doing something different from what the actual system writes to the column, so it seems a little scary to decouple it from how a pts record will actually be written?
There was a problem hiding this comment.
Hm, I'm not sure, but I see your point. I was coming at it to minimize the test/make it fast enough to also then suggest writing a benchmark for it ("TestProtectedTimestampDecoder only cares about decoding"). Minimizing the test also prevents it from failing when another subsystem is buggy (Protect/Release); the nice thing about talking through this table is that these sub-systems are decoupled, I think it's fine/sane to have them decoupled in test code as well. I'll defer to you and not hold up the PR.
There was a problem hiding this comment.
Minimizing the test also prevents it from failing when another subsystem is buggy
Coming from bulkio land where we write almost all tests that rely on the jobs subsystem, this is something I've just taken for granted before heh. The benchmarking is a good point, I'm leaning towards merging this for now since I have CI happy, and cleaning it up during stability if we decide to benchmark some of these pieces.
This change adds a decoder to decode rows from the `system.protected_ts_records` table. This will be used by the rangefeed we setup in the SQLWatcher, to decode rangefeed updates and subsequently feed them into the SQLTranslator. The `target` column of the system table is the only column of interest since it will determine whether we ask the translator to generate `SystemSpanConfig`s or regular `SpanConfigs`. More information about this will be added in follow up PRs that use this decoder. This change also instructs gogoproto to generate an equality method for `roachpb.TenantID`, to allow for equality checks in the decoder test. Informs: cockroachdb#73727 Release note: None
2793d85 to
5a18c3a
Compare
|
TFTR! bors r=irfansharif |
|
Build succeeded: |
This change adds a decoder to decode rows from the
system.protected_ts_recordstable. This will be used by therangefeed we setup in the SQLWatcher, to decode rangefeed updates
and subsequently feed them into the SQLTranslator. The
targetcolumnof the system table is the only column of interest since it will
determine whether we ask the translator to generate
SystemSpanConfigsor regular
SpanConfigs. More information about this will be addedin follow up PRs that use this decoder.
This change also instructs gogoproto to generate an equality method
for
roachpb.TenantID, to allow for equality checks in the decodertest.
Informs: #73727
Release note: None