fix: pipe metadata along #1178
fix: pipe metadata along #1178
Conversation
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
…nodejs-bigtable into metadata-bubble-up
…nodejs-bigtable into metadata-bubble-up
There was a problem hiding this comment.
This is looking like a good start. 👍 I'm curious to understand better what is the mechanism that makes it so that the emitted metadata makes it way into the mutationErrorsByEntryIndex map.
Given that the test is always throwing at the moment I'm not entirely convinced that the implementation is working as intended. Let's start with improving the tests and feel free to elaborate a bit more on the mechanism!
Having working tests that assert the exact expected values for the metadata will make me more confident on this PR 😊
system-test/bigtable.ts
Outdated
| await TABLE.insert(rows); | ||
| assert.fail('Inserts should not work on non-existent column family.'); |
There was a problem hiding this comment.
assert.fail is always going to be throwing an AssertionError thus always triggering the following catch block
| await TABLE.insert(rows); | |
| assert.fail('Inserts should not work on non-existent column family.'); | |
| // Inserts should not work on non-existent column family. | |
| await TABLE.insert(rows); |
That message will live better as a comment or something similar
There was a problem hiding this comment.
Are you sure? I want the test to fail if the insert succeeds. In this case it will go to the catch block, but then the other assertion will fail.
system-test/bigtable.ts
Outdated
| await TABLE.insert(rows); | ||
| assert.fail('Inserts should not work on non-existent column family.'); | ||
| } catch (e: any) { | ||
| assert(e.metadata); |
There was a problem hiding this comment.
I believe it's better to try and match the exact expected values of this metadata object here rather than just a blank non-falsy assertion.
There was a problem hiding this comment.
Added a check for two of the metadata properties that don't change.
src/table.ts
Outdated
| @@ -1612,6 +1614,9 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); | |||
| mutationErrorsByEntryIndex.set(originalEntriesIndex, errorDetails); | |||
| }); | |||
| }) | |||
| .on('metadata', metadata => { | |||
There was a problem hiding this comment.
I don't think we need to save the metadata for now. When we were debugging the customer's issue, we wanted to see the value of server-timing that's published by GFE. In the future we'll also add this as a metric. But for now I think the changes in tables.ts could be reverted.
There was a problem hiding this comment.
Ok. I guess the original intention was to make it so that the metadata is available on the stream, but doesn't need to reach the user?
Work towards #991