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

Questions about Direct Access Feature #5442

Closed
dplewis opened this issue Mar 21, 2019 · 19 comments · Fixed by #5550
Closed

Questions about Direct Access Feature #5442

dplewis opened this issue Mar 21, 2019 · 19 comments · Fixed by #5550
Labels
type:docs Only change in the docs or README

Comments

@dplewis
Copy link
Member

dplewis commented Mar 21, 2019

Enables direct access to parse-server, when using the JS SDK from the current node runtime.

All API calls from the JS-SDK are router through route matching (but /functions/:functionName)

Enable that feature by setting PARSE_SERVER_ENABLE_EXPERIMENTAL_DIRECT_ACCESS=1

So if I understand this correctly:

client -> request to Cloud Code Function (Server) -> calling object.save() in Cloud Code -> send http request to Server.

Enabling this feature

client -> request to Cloud Code Function (Server) -> calling object.save() in Cloud Code -> no additional request sent because both JS SDK and Server are on the same node runtime

Enabling this feature would make requests faster since there is one less request, should this be mainstreamed?

Should there be tests run against this?

Add to parse-server config definitions?

We should add documentation to make more users aware of this feature

@acinader
Copy link
Contributor

acinader commented Mar 21, 2019

or we should just turn it on?

I've never tested it which i'll try and do real soon now....

I don't know what potential issue if any it may cause. if there's not breaking change to it, then we should just turn it on. I think that @flovilmart wanted it out there to figure out if there was a breakage issue iircc ...

BUT, adding it to the definitions and testing would be good either way.

@flovilmart
Copy link
Contributor

Most of the Test break with this feature on. There can be bad side effects due to the statefulness of the JS SDK.

@acinader
Copy link
Contributor

In which case, @dplewis your initial suggestion is spot on.

@flovilmart
Copy link
Contributor

I mean the whole test suite is broken when using this feature

Sent with GitHawk

@dplewis
Copy link
Member Author

dplewis commented Mar 22, 2019

I haven’t used it yet and don’t know the limitations but would love too. At least start testing against different routing matches and see how responses are handled

@flovilmart
Copy link
Contributor

Simple thing is to turn it on for the test suite, and see what breaks. In an ideal world, all tests should pass as it warrants all cloud code / integration tests are OK.

Sent with GitHawk

@dplewis
Copy link
Member Author

dplewis commented Mar 22, 2019

It’s a great feature that is known by few. Definitely could use feedback. I’ll test it out shortly.

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2020

@dplewis or @davimacedo do you know what the state of this feature is?

It's feature flagged, merged into master but a lot of test cases still fail, so not safe for production, is that correct?

@mman
Copy link
Contributor

mman commented Apr 22, 2020

@mtrezza i have it in production for more than a year, since 2.8 IIRC, if I understand it correctly requests from cloud code go directly to the instance handling the original request as opposed going outside to the load balancer doing TLS and back inside again. Not checked the failing tests though...

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2020

@mman Thanks. Do you recall the reason why you took it into production? Did you do a performance comparison?

Sent with GitHawk

@mman
Copy link
Contributor

mman commented Apr 22, 2020

@mtrezza I think it was because I had a lot of performance and memory problems with the 2.x series, I do not remember exactly nowadays, I did not know much about the code then (not that it has change too much :))), but one of them was single schema cache, parse server would leak memory without it, and push notifications were slow so I opted in for direct access. I also used experimental feature to delete invalid tokens, but that one was removing for me valid tokens at times, but single schema cache and direct access remained in my deployment and everything seems working fine. Also, since upgrading to 4.x memory consumption and performance characteristics changed drastically, 4.x is much faster in serving requests (Note: I have skipped 3.x entirely as I needed to rewrite a lot of cloud code and it took me too much time).

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2020

@mman Thanks for the details.

Regarding the invalid push token cleanup, I saw a significant resource improvement after activating it, especially DB side, because each push would increase the installation push notifications count, which was unnecessary for invalid tokens, causing DB writes. You may want to read my analysis in #6052 and decide whether it is production ready in your setting. I should do a PR for it soon so we can remove the experimental flag.

Sent with GitHawk

@davimacedo
Copy link
Member

@mtrezza I have this feature activated for some of my apps with huge performance improvement in terms of memory and latency. I believed we should switch this option to be activated by default but you've just raised that some test cases are failing. Where did you see they failing? Would you mind to open a PR with this option set true by default so we can see them failing and work together to fix it?

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2020

@davimacedo I only inferred failing test cases from the conversion in #5550. It looks as if the experimental feature was merged despite failing cases to make it available and get more insight. Maybe @dplewis can clarify this.

Yes, let's team up and make this enabled by default, will open a PR and see if there are any failing test cases.

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2020

@davimacedo Can you please reopen this issue? I will start a new issue, as this issue was apparently about adding and documenting the feature flag.

@davimacedo
Copy link
Member

Sure thing.

@davimacedo davimacedo reopened this Apr 22, 2020
@davimacedo
Copy link
Member

Ok. Closing again :)

@mtrezza
Copy link
Member

mtrezza commented Apr 22, 2020

@davimacedo Thanks. My apologies :)

@mman
Copy link
Contributor

mman commented Apr 23, 2020

@mtrezza I'm following #6052 closely and am ready to give it another try :)

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.

6 participants