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

[9.2] Before a user is getting scanned the database connection is re-establ… #25853

Merged

Conversation

@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Aug 18, 2016

…ished

refs #25480

…ished
@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Aug 18, 2016

@DeepDiver1975, thanks for your PR! By analyzing the annotation information on this pull request, we identified @icewind1991, @mmattel and @bartv2 to be potential reviewers

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Aug 18, 2016
}
while (!$connection->isConnected()) {
try {
$connection->connect();

This comment has been minimized.

Copy link
@PVince81

PVince81 Aug 18, 2016

Member

please add a max retries count to avoid infinite loops

This comment has been minimized.

Copy link
@PVince81

PVince81 Aug 18, 2016

Member

in which case the exception is rethrown to the outside

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Aug 18, 2016

Author Member

I'd leave this to the admin then to decide if/when he wants to terminate the scan

This comment has been minimized.

Copy link
@PVince81

PVince81 Aug 18, 2016

Member

Ah yes, in my mind I saw this method as part of DBConnection but it isn't. Since this loop is only in the context of file scanning then it's fine.

@DeepDiver1975 DeepDiver1975 changed the title Before a user is getting scanned the database connection is re-establ… [9.2] Before a user is getting scanned the database connection is re-establ… Aug 18, 2016
@georgehrke

This comment has been minimized.

Copy link
Contributor

georgehrke commented Aug 18, 2016

👍

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 19, 2016

@DeepDiver1975 any chance to have a little unit test that covers the reconnection part, so that CI can tell us that all databases are ok with it ?

@DeepDiver1975

This comment has been minimized.

Copy link
Member Author

DeepDiver1975 commented Aug 19, 2016

so that CI can tell us that all databases are ok with it ?

there is not much dbms specific in here - doctrine unsets the connection on close and creates a new connection within connect.

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 19, 2016

Another concern of mine is whether reconnecting a thousand times is slower than keeping the connection open. If slower, a possible strategy would be to keep track of elapsed time and after every user check if the time is more than a minute or so. If yes, then reconnecting.

But if you think the overhead is negligible then I'm fine with this.

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 22, 2016

@DeepDiver1975 can you check my previous comment ? Would be good to get this merged and backported soon, unless you think this is too risky

@DeepDiver1975

This comment has been minimized.

Copy link
Member Author

DeepDiver1975 commented Aug 22, 2016

But if you think the overhead is negligible then I'm fine with this.

Form my pov this can be ignored

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 22, 2016

👍 tested with LDAP and 100 users

@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 22, 2016

Jenkins unpublished success message -> merge

@PVince81 PVince81 merged commit 27a5be9 into master Aug 22, 2016
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/jenkins/pr This commit is being built
Details
Scrutinizer 1 new issues, 2 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the files-scan-reconnect-database-before-each-user-master branch Aug 22, 2016
@PVince81

This comment has been minimized.

Copy link
Member

PVince81 commented Aug 22, 2016

@DeepDiver1975 please send the backports, thanks

@DeepDiver1975

This comment has been minimized.

Copy link
Member Author

DeepDiver1975 commented Aug 22, 2016

will backport

@lock

This comment has been minimized.

Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.