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

Refactor QgsConnectionPool[Group] and it's descendants #53307

Closed

Conversation

reflectored
Copy link

@reflectored reflectored commented May 31, 2023

Description

This refactoring job is related to qgis/QGIS-Enhancement-Proposals#272 and the original pull request for the AWS Redshift driver #53018.

  • Move qgsConnectionPool_[connectionCreate|connectionDestroy|invalidateConnection|connectionIsValid|connectionToName] into their appropriate abstract classes and declare them as pure virual.
  • Implement the destructor and the functions above for all providers.
  • Refactor QgsPostgresConnPool[Group] to QgsSQLConnPool[Group], in order to reuse the template for future SQL based providers (AWS Redshift).
  • Refactor Postgres connection settings to use the latest settings API.

@reflectored
Copy link
Author

@nyalldawson @jef-n I started the refactoring work we have discussed. I noticed some redundant code with the connection pool so I started here.

@github-actions github-actions bot added this to the 3.32.0 milestone May 31, 2023
@reflectored reflectored force-pushed the rebase_provider_conn_pool branch 2 times, most recently from 4d4ab90 to 61576e9 Compare June 1, 2023 12:35
src/core/qgsconnectionpool.h Outdated Show resolved Hide resolved
Copy link
Contributor

@elpaso elpaso left a comment

Choose a reason for hiding this comment

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

Thank you for working on this refactoring, it was definitely time to do it.

@nyalldawson nyalldawson added the Frozen Feature freeze - Do not merge! label Jun 1, 2023
@reflectored reflectored force-pushed the rebase_provider_conn_pool branch 4 times, most recently from fcf9375 to aa07b48 Compare June 2, 2023 12:01
@reflectored
Copy link
Author

@elpaso thanks for the review, I have addressed all of your comments!

src/core/qgsconnectionpool.h Outdated Show resolved Hide resolved
@reflectored
Copy link
Author

@elpaso not sure if the spell_check error is related. Would you mind taking another look?

@elpaso
Copy link
Contributor

elpaso commented Jun 13, 2023

@elpaso not sure if the spell_check error is related. Would you mind taking another look?

Looks good to me, I'll try to restart the failing jobs, sometimes it helps (even if it shouldn't).

@3nids
Copy link
Member

3nids commented Jul 20, 2023

My preferred approach would indeed be the duplication of the settings. Indeed chances of divergence are there and it's better isolated and also avoid the need for the dynamic naming.

@reflectored
Copy link
Author

Also, my solution did not work for the tests and mingw build, the same QgsSettingsException was thrown during the provider test runs - resolved to normal settings class.

@reflectored
Copy link
Author

reflectored commented Jul 24, 2023

@3nids something seems off with the CI, when I run locally the tests e.g. ALL_BUT_PROVIDERS, I do not hit these failures anymore on my latest patch (due to QgsSettingsException). Only some flaky tests fail for other reasons:

98% tests passed, 18 tests failed out of 855

Label Time Summary:
POSTGRES    =   8.31 sec*proc (3 tests)

Total Test time (real) = 2632.87 sec

The following tests FAILED:
	  2 - checkGitStatus (Failed)
	 35 - test_core_authmanager (Failed)
	135 - test_core_networkaccessmanager (Subprocess aborted)
	222 - test_core_openclutils (Subprocess aborted)
	278 - test_gui_filedownloader (Subprocess aborted)
	296 - test_gui_queryresultwidget (Subprocess aborted)
	300 - test_3d_3drendering (Failed)
	324 - test_analysis_processing (Failed)
	328 - test_provider_wcsprovider (Failed)
	482 - PyQgsExternalStorageWebDav (Failed)
	483 - PyQgsExternalStorageAwsS3 (Failed)
	600 - PyQgsMapLayerComboBox (Failed)
	603 - PyQgsMapLayerProxyModel (Failed)
	664 - PyQgsProcessExecutablePt1 (Failed)
	665 - PyQgsProcessExecutablePt2 (Failed)
	769 - PyQgsStyleModel (Failed)
	852 - qgis_sipify (Failed)
	854 - qgis_sip_uptodate (Failed)
Errors while running CTest
Output from these tests are in: /home/qgis/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

Also I struggle to get a container for the MinGW working, I tried manually running on fedora:rawhide, but then the NodeJS version there is too new for the current configurations. Do you have a particular container for MinGW at hand? Got to reproduce the build failure for MinGW, it is missing symbols for PostgreSqlConnectionSettings - I'm investigating.

@reflectored reflectored force-pushed the rebase_provider_conn_pool branch 2 times, most recently from 27a9431 to 3a2dfc1 Compare July 25, 2023 16:13
Copy link
Member

@3nids 3nids left a comment

Choose a reason for hiding this comment

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

on the settings front, everything looks good!
thanks a lot for the changes.

@reflectored reflectored requested a review from elpaso July 26, 2023 10:44
@reflectored reflectored force-pushed the rebase_provider_conn_pool branch 7 times, most recently from 2361cb3 to 48cbe44 Compare July 28, 2023 13:36
Alexey Karandashev added 4 commits July 31, 2023 09:18
…and the original pull request for the AWS Redshift driver qgis#53018.

  - Move qgsConnectionPool_[connectionCreate|connectionDestroy|invalidateConnection|connectionIsValid|connectionToName] into their appropriate abstract classes and declare them as pure virual.
  - Implement the destructor and the functions above for all providers.
  - Refactor QgsPostgresConnPool[Group] to QgsSQLConnPool[Group], in order to reuse the template for future SQL based providers (AWS Redshift).
@reflectored
Copy link
Author

@elpaso it passes all tests now! I appreciate your review.

@reflectored
Copy link
Author

ping @elpaso

@elpaso
Copy link
Contributor

elpaso commented Feb 22, 2024

ping @elpaso

Sorry for the late reply, can you please resolve the merge conflicts?

@nyalldawson nyalldawson removed the Frozen Feature freeze - Do not merge! label Apr 5, 2024
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Apr 20, 2024
Copy link

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@github-actions github-actions bot closed this Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HANA data provider PostGIS data provider stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants