Skip to content

Update PostgresStorageAdapter.js #3578

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

Merged
merged 7 commits into from
Mar 4, 2017
Merged

Update PostgresStorageAdapter.js #3578

merged 7 commits into from
Mar 4, 2017

Conversation

vitaly-t
Copy link
Contributor

@vitaly-t vitaly-t commented Feb 27, 2017

proper database API usage, via transaction.

proper database API, via transaction.
@facebook-github-bot
Copy link

@vitaly-t updated the pull request - view changes

adding the same rejection approach as before, the functionality remains identical.
@facebook-github-bot
Copy link

@vitaly-t updated the pull request - view changes

return t.batch(promises);
})
.then(data => {
debug(`initialzationDone in ${data.duration}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: should be initialization

return Promise.all(promises).then(() => {
debug(`initialzationDone in ${new Date().getTime() - now}`);
}, () => {});
.catch(error => {
Copy link
Contributor

Choose a reason for hiding this comment

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

hm. so if you catch the error and then do nothing with it is that the right thing? will parse server fail? should it?

i'm assuming you're using console instead of logger cause we can't know that the logger is available yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The end result after the change should be identical to what it was, except the integrity is ensured + using just one connection.

Copy link
Contributor

@acinader acinader left a comment

Choose a reason for hiding this comment

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

looks good to me, nit typo and a question for you....

@facebook-github-bot
Copy link

@vitaly-t updated the pull request - view changes

@facebook-github-bot
Copy link

@vitaly-t updated the pull request - view changes

@facebook-github-bot
Copy link

@vitaly-t updated the pull request - view changes

@vitaly-t
Copy link
Contributor Author

vitaly-t commented Mar 4, 2017

After a few tweaks it is good for merging now ;)

@facebook-github-bot
Copy link

@vitaly-t updated the pull request - view changes

@acinader acinader merged commit 271608b into parse-community:master Mar 4, 2017
@vitaly-t vitaly-t deleted the patch-1 branch March 5, 2017 10:12
@acinader
Copy link
Contributor

acinader commented Mar 5, 2017

@vitaly-t hm so this may be problematic it appears.

  1. lots of console out put in tests that would be good to do without.
  2. failing postgress tests. probably just flaky as re-running makes them pass, but would be good to see how to make more reliable?

@vitaly-t
Copy link
Contributor Author

vitaly-t commented Mar 5, 2017

lots of console out put in tests that would be good to do without.

I only repeated what was written into the console previously.

failing postgress tests

Which tests? As far as I can see they all succeed.

@acinader
Copy link
Contributor

acinader commented Mar 5, 2017

https://travis-ci.org/ParsePlatform/parse-server/jobs/207943837

so there's the output issue. i do know that this wasn't showing up before.

bad me! I've just be re-running failing tests to get them to pass. would be good if i could figure out how to highlight flakey tests....

in any event, if any fail today, i wont rerun or at least i'll catch the output and put it here.

@vitaly-t
Copy link
Contributor Author

vitaly-t commented Mar 5, 2017

That, plus the code isn't very solid. It doesn't use transactions where it should, so often data or structure become inconsistent.

My change was to start fixing that, but it requires more than that to fix the whole, to patch the data integrity.

To that end you have all the right tools, transactions via pg-promise are trivial to implement, just takes good understanding of the existing code, how to make the change right ;)

@acinader
Copy link
Contributor

acinader commented Mar 5, 2017

can you look into quieting the output in the tests if it isn't an issue?

@vitaly-t
Copy link
Contributor Author

vitaly-t commented Mar 5, 2017

What you see there are errors, clearly an issue. You simply output the error, which is of type BatchError, which provides verbose object inspection. Nothing to switch off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants