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

Hotfix export timeout issue #261

Closed
wants to merge 3 commits into from
Closed

Conversation

yaozhihang
Copy link
Member

Fix the issue #257.

It seems that the issue is caused by the very long where clause in the root select statement. So, putting the long where clause in a subquery solves the issue.

@yaozhihang yaozhihang changed the base branch from master to release-5.2 July 21, 2022 13:30
@yaozhihang yaozhihang requested a review from clausnagel July 21, 2022 13:30
@yaozhihang yaozhihang mentioned this pull request Jul 21, 2022
@BWibo
Copy link
Member

BWibo commented Jul 22, 2022

@yaozhihang I just created a fresh build and tested this with the workaround described in #257 (comment) enabled and disabled and can confirm this works! Thx for the quick fix.

@clausnagel
Copy link
Member

@yaozhihang, thanks for proposing this hotfix to solve #257. However, I'm not sure whether this is really a "fix". Imho, the query optimizer should deal with "long" where clauses automatically, not the application code. Rearranging the query in the application code as proposed might help in the context of #257, but might have no (or even a bad) impact in other scenarios. For instance, the getNumberMatched method in DBSplitter still uses the unchanged query and yet performs good.

And does your change affect exports from Oracle?

@BWibo, have you already tried and "vacuumed" your database? Maybe this already helps?

Zhihang Yao added 2 commits July 25, 2022 11:05
@clausnagel
Copy link
Member

@BWibo, thanks for vacuuming the database. Unfortunately, I again ran into the issue in my export test. So, it seems that vacuuming might help but does not solve the issue.

@clausnagel
Copy link
Member

clausnagel commented Jul 25, 2022

When adding DatabaseConnectionPool.getInstance().purge(); as line 591 to the class org.citydb.core.operation.exporter.controller.Exporter (so, as last line of the finally block), the export also completes without issues in my tests.

This seems to support that re-arranging the query is not necessary. As far as I understand, purge() closes all idle connections. This is of course not what we want in the first place, because we use a pool to avoid having to physically open connections all the time. But still it shows that there seems to be an issue with pool maintenance and handling of idle connections (maybe even a Tomcat JDBC pool bug?).

@yaozhihang
Copy link
Member Author

clausnagel's solution looks better. It ensures that the pool produces clean connections for each export.

@clausnagel
Copy link
Member

clausnagel commented Jul 26, 2022

It seems that PR #264 also solves the issue #257 and thus makes this PR obsolete. @yaozhihang and @BWibo, can you please test and confirm?

@clausnagel
Copy link
Member

Discarding this PR due to #263

@clausnagel clausnagel closed this Jul 27, 2022
@clausnagel clausnagel deleted the hotfix-export-timeout branch July 27, 2022 08:29
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