Skip to content
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

missing docs for setting the readPreference option in a query #5515

Closed
oallouch opened this issue Apr 16, 2019 · 18 comments · Fixed by parse-community/docs#635
Closed

missing docs for setting the readPreference option in a query #5515

oallouch opened this issue Apr 16, 2019 · 18 comments · Fixed by parse-community/docs#635
Labels
type:docs Only change in the docs or README

Comments

@oallouch
Copy link
Contributor

oallouch commented Apr 16, 2019

Hi,
Like in my previous issue (where I said I can't find user.getSessionToken() in the docs), I face another documentation problem : this time I'd like to make some queries read from the secondary mongodb server, but I can't find the readPreference in the reference js documentation.

Thx a lot in advance,
Olivier

@dplewis dplewis added type:docs Only change in the docs or README help wanted labels Apr 18, 2019
@davimacedo
Copy link
Member

The only way to change the readPreference so far is by using a beforeFind trigger, like in the example below:

Parse.Cloud.beforeFind('MyClass', req => {
  req.readPreference = 'SECONDARY';
});

Note that in this example all queries in MyClass will be changed to the SECONDARY, but you can also do some verifications in the req object in order to check for a specific set of queries and only change for them.

I will open a PR soon in the docs repo explaining this.

@oallouch
Copy link
Contributor Author

That's odd. Reading the sources, I would think I can do it directly in a Query

case 'readPreference':

and MongoStorageAdapter seems to retrieve it from the Query, next to limit, skip, ...
{ skip, limit, sort, keys, readPreference }: QueryOptions

What do you think ?

@dplewis
Copy link
Member

dplewis commented Apr 26, 2019

@oallouch Good catch surprised query.setReadPerference() was never added. Would you like to do a PR?

@oallouch
Copy link
Contributor Author

I can try, but it'll be my first. I might need some help.
For instance, when I try npm install , I get many errors like:

npm WARN tar ENOENT: no such file or directory, open 'C:\Users\xxx\Documents\GitHub\Parse-SDK-JS\node_modules.staging\insert-module-globals-cbc4cf8a\test\global\filename.js'

@dplewis
Copy link
Member

dplewis commented Apr 26, 2019

Which node version are you running and do you have permissions?

@davimacedo
Copy link
Member

davimacedo commented Apr 26, 2019

@dplewis @oallouch when I wrote the readPreference option, my original idea was to include it as an option in the API and also in the JavaScript SDK, but, in that point in time, there was a discussion among some of the maintainers and they thought it was not a good idea to allow the client to choose where the query should run (it is a backend responsibility). I agree with the argument but I still think it would be much easier to have this option in the API. We can also protect it with master key so it could be set only in cloud code and server environments. So I see we have three options:

  • keep like it is, only setting it using triggers
  • include the readPreference option in the API and SDKs without requiring a master key
  • include the readPreference option in the API and SDKs requiring master key

What do you think? @acinader can you please also share your thoughts on this?

@davimacedo
Copy link
Member

I've just remembered we actually wrote the PR with the API option for readPreference but it was never approved:
#3963

I think it is a good start point for a new PR on this.

@oallouch
Copy link
Contributor Author

The second one is very fine with me.

@oallouch
Copy link
Contributor Author

@davimacedo I think you should write the server and client PRs. All the options (readPreference, includedReadPreference and subqueryReadPreference ) aren't as clear for me as it must be for you.

Btw, when a readPreference is set for a query, shouldn't it also change the default values of includedReadPreference and subqueryReadPreference ?

@dplewis my problem was just a config issue

@oallouch
Copy link
Contributor Author

oallouch commented May 6, 2019

@dplewis I'm trying to write a Query.setReadPreference() test in the JS SDK, but I can't find an example of test that uses the output REST call generated from a Query.
I looked in integration/test/ParseQueryTest.js

@oallouch
Copy link
Contributor Author

Ok, I'll just look at the output of ParseQuery.toJSON() .

@dplewis
Copy link
Member

dplewis commented May 11, 2019

It would be easier to wait for something like #3963 to be implemented. That PR needs rebasing. @davimacedo Would you like to take a crack at it again?

@oallouch
Copy link
Contributor Author

Yes, that would be good.
#3963 is great, but 'includeReadPreference' and 'subqueryReadPreference' aren't completely treated in MongoStorageAdapter

I tried to modify the server myself.
I added the symlinks (using npm link) so that my project uses the local parse-server repo (with express), but I get "Cannot find module 'parse/node'".
Any ideas ?

@oallouch
Copy link
Contributor Author

Never mind, I did
const Parse = require('parse/node').Parse;
in a file.
It worked with the normal release, but not with this local symlink.

@davimacedo
Copy link
Member

@dplewis @oallouch I will rebase the PR this Monday and let you guys know.

@davimacedo
Copy link
Member

The API changes were merged. I will now work on JS SDK.

@TomWFox
Copy link
Contributor

TomWFox commented Jul 6, 2019

For the sake of clarity the API changes were made in #3963, @davimacedo I believe changes to the JS SDK are still to be done? - maybe a separate issue should be opened there and this one be closed?

@davimacedo
Copy link
Member

@TomWFox we still need to add the read preference option in the JS SDK and updated the docs. I will work this Tuesday to close the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:docs Only change in the docs or README
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants