Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support for using multiple databases in datastore #1090
feat: Support for using multiple databases in datastore #1090
Changes from 50 commits
11524c1
e06bee0
1f464a5
10da9c8
ecfce7d
3b6a42c
e254be5
77970c8
3b86ce2
ea54946
38d955c
57bc2eb
c9856b2
543e486
dde1eba
8ba512a
de946b2
dd7b06c
f167ce3
364845b
7b5f902
a3bdb92
9d7fab1
d9a96bf
7543f57
3f3e965
32bb92b
92f8778
6159882
9410652
b34bf17
2e1999a
bd97d00
d41ca60
1519ad3
0ebabd5
6449d10
e9ee522
1ba9756
d35cdee
769417d
e694341
f307f4d
0f27ea4
76a3dfc
3fd19fb
d4e3f91
771390c
ae15bc4
6b5c542
da64dab
8d170fd
d49e5fc
e7082ab
b50b065
f3aadc2
65605e8
6986d4c
0378dea
a190512
4b62317
88e20ed
739796f
87450ef
28b5aaa
46a5627
2cf3d98
9505083
e34735a
eb55764
992117f
7312f49
aef80a1
72255ef
07dc746
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here and not along side wherever
reqOpts.projectId
lives? (looking atrequest.ts
, line 1030)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
databaseId
is inRequestOptions
becausereqOpts
is of typeRequestOptions
andreqOpts
is what gets sent here to the backend.projectId
lives insharedQueryOptions
so therefore also lives inRequestOptions
by inheritance which is why we can attachprojectId
on line 1030. Now the reasondatabaseId
should not live insharedQueryOptions
is because then they would be included inRunAggregationQueryRequest
, but they do not need to be. I am not sure whyprojectId
lives insharedQueryOptions
and perhaps that is something that needs to be investigated further.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline, will move
databaseId
to shared query options to match proto: example https://github.com/googleapis/googleapis/blob/master/google/datastore/v1/datastore.proto#L208-L234There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like it's only used in one place in the (non test) code. Is this necessary, or can we simply inline it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could inline it, move this to
test/util.ts
so that multiple test files can use this shared code and then add a comment to say that the same logic is in two places.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second thought, I really think we should avoid having duplicate code in two places (this logic in the test code and this logic in request.ts). So I think we should keep this function here and call it directly from both places instead of going through
addDatabaseIdToRequest
onindex.ts
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add some more comments in tests specifically