Conversation
Fixed the BSON parsing bug that was hidden by the KS state store caching mechanism. Without the proper @BsonCreator constructor, our document retrieval and parsing from MongoDB were never getting their ID fields populated correctly.
agavra
left a comment
agavra
left a comment
There was a problem hiding this comment.
Nice fix! Thanks @apourchet
| public WindowDoc() { | ||
| } | ||
|
|
||
| @BsonCreator |
There was a problem hiding this comment.
do we need to do the same for KVDoc? I can't remember if you said that's going to be a separate PR or not. Also for KVDoc I noticed there's @BsonId annotation on the id field, is that necessary/needed?
There was a problem hiding this comment.
I did some testing on KVDoc independently and it looks like @BsonId isn't doing what we want it to. I wanted to see what the feedback was on this Window stuff before applying the same logic to the KVDoc (since it's already in use in production).
There was a problem hiding this comment.
note: none of the Mongo stuff is in production yet, the KV store that we have in prod is based on Scylla. So now is the time to make any changes we want/need
| var kvs = new ArrayList<KeyValue<WindowedKey, byte[]>>(); | ||
| it.forEachRemaining(kvs::add); | ||
|
|
||
| assertThat(kvs, Matchers.hasSize(1)); |
There was a problem hiding this comment.
nit: since we're trying to fetch a range, can we verify that multiple values can be returned instead of just one?
I know that's unrelated to this PR, but since this is somewhat of an integration test I think it's worth covering that part as well
| it.forEachRemaining(kvs::add); | ||
|
|
||
| assertThat(kvs, Matchers.hasSize(1)); | ||
| assertThat(kvs.get(0).key.key, Matchers.equalTo(windowedKey.key)); |
There was a problem hiding this comment.
was this the thing that was failing on main?
There was a problem hiding this comment.
Yep, it's the range test that's failing on main. The reason behind the failure is that the WindowKey was being populated using the .id field, which throws NullPointer as it's never populated by the Bson decoder.
| @BsonProperty(VALUE) byte[] value, | ||
| @BsonProperty(EPOCH) long epoch | ||
| ) { | ||
| this.id = id; |
There was a problem hiding this comment.
Just want to make sure I understand the actual fix here -- I did notice the this.id = id was missing and added that in #251 but IIUC you're saying that even with this, we still have a bug -- due to missing Bson annotations, essentially?
There was a problem hiding this comment.
I just tested this locally, which goes back to failing the new test:
public class WindowDoc {
@@ -43,11 +41,10 @@ public class WindowDoc {
public WindowDoc() {
}
- @BsonCreator
public WindowDoc(
- @BsonProperty(ID) BasicDBObject id,
- @BsonProperty(VALUE) byte[] value,
- @BsonProperty(EPOCH) long epoch
+ BasicDBObject id,
+ byte[] value,
+ long epoch
) {
this.id = id;
this.value = value;
So it looks like the Bson decoder needs the actual annotations and not just the right constructor signature.
ableegoldman
left a comment
ableegoldman
left a comment
There was a problem hiding this comment.
LGTM -- still curious why this bug surfaced for session stores but not the others. I did check and our integration tests do a flush on every record, so (un?)fortunately I don't think the CommitBuffer serving all the records in the tests is the answer.
I believe the reason we didn't catch this earlier is that none of the integration tests call out to the |
3 participants
Fixed the BSON parsing bug that was not yet hit by the integration tests. Without the proper @BsonCreator constructor, our document retrieval and parsing from MongoDB were never getting their ID fields populated correctly.