Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix: pipe metadata along #1178

Merged
merged 21 commits into from
Jan 27, 2023
Merged

fix: pipe metadata along #1178

merged 21 commits into from
Jan 27, 2023

Conversation

danieljbruce
Copy link
Contributor

Work towards #991

@danieljbruce danieljbruce requested review from a team as code owners September 23, 2022 21:10
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Sep 23, 2022
@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Sep 23, 2022 •
edited
Loading

🤖 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 automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Sep 23, 2022
@danieljbruce danieljbruce marked this pull request as draft September 23, 2022 21:10
@danieljbruce danieljbruce mentioned this pull request Sep 23, 2022
Closed
@danieljbruce danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 26, 2022
@danieljbruce danieljbruce marked this pull request as ready for review September 27, 2022 13:34
@danieljbruce danieljbruce added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Sep 27, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 27, 2022
@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2022
@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2022
ruyadorno
ruyadorno reviewed Sep 27, 2022
View reviewed changes
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment on lines 713 to 714
await TABLE.insert(rows);
assert.fail('Inserts should not work on non-existent column family.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.fail is always going to be throwing an AssertionError thus always triggering the following catch block

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a check for two of the metadata properties that don't change.

@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 27, 2022
mutianf
mutianf requested changes Sep 27, 2022
View reviewed changes
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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. and removed size: s Pull request size is small. labels Sep 28, 2022
@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 28, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 28, 2022
@danieljbruce danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 27, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 27, 2023
@danieljbruce danieljbruce merged commit 0822e4d into googleapis:main Jan 27, 2023
@release-please release-please bot mentioned this pull request Jan 27, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@ruyadorno ruyadorno ruyadorno left review comments

@mutianf mutianf mutianf approved these changes

Assignees
No one assigned
Labels
api: bigtable Issues related to the googleapis/nodejs-bigtable API. size: xs Pull request size is extra small.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@danieljbruce @ruyadorno @mutianf @yoshi-kokoro