Skip to content

Comments

CI for Oracle DB#9862

Closed
FaritSlv wants to merge 38 commits intodoctrine:2.13.xfrom
FaritSlv:oracle
Closed

CI for Oracle DB#9862
FaritSlv wants to merge 38 commits intodoctrine:2.13.xfrom
FaritSlv:oracle

Conversation

@FaritSlv
Copy link

Try running the tests on Oracle

Co-authored-by: Alexander M. Turek <me@derrabus.de>
@FaritSlv
Copy link
Author

FaritSlv commented Jul 6, 2022

I tried several options. Running out of ideas

On the local, I found out that the Oracle DB at some point starts blocking requests or hangs up due to a large number of reads and writes. Tried to disable all possible logging. Autocommits.

The test passes on the github, because the process hangs for a long time, then the database comes to life and goes on. And so several times, because of this for so long

@greg0ire, any ideas where else to go?

@derrabus derrabus marked this pull request as ready for review July 6, 2022 10:26
@FaritSlv
Copy link
Author

FaritSlv commented Jul 6, 2022

image

Stuck on the 1080th test, eventually restarted the database container

@FaritSlv
Copy link
Author

FaritSlv commented Jul 6, 2022

Individually, all tests pass quickly.

@greg0ire
Copy link
Member

greg0ire commented Jul 6, 2022

@greg0ire, any ideas where else to go?

No, the only experience with Oracle I have is with the PR you started from.

@derrabus derrabus marked this pull request as draft July 7, 2022 08:14
@FaritSlv
Copy link
Author

FaritSlv commented Jul 10, 2022

Hi
@derrabus, @morozov, @greg0ire

After adding this fix doctrine/dbal#768, there may have been a profit, but only if you run each test separately. But when all the tests are done, it eats up all the resources of Oracle, and this is very sensitive for the Express Edition. If you return it to joins, then the tests will pass even locally. There is something to think about, for example, tests pass more or less quickly, but one test freezes for 40 minutes "\Doctrine\Tests\ORM\Functional\SchemaTool\CompanySchemaTest::testGeneratedSchema", as I understand it, the bottleneck is getting metadata. I think Oracleplatform can be improved in dbal

@morozov
Copy link
Member

morozov commented Jul 10, 2022

The optimization made in doctrine/dbal#768 may be irrelevant as of doctrine/dbal#5268. The Oracle's system views indeed take a while to initialize but this has been mitigated by querying them once for the entire schema introspection as opposed to querying them once per table.

We can consider reverting the previous optimization and using joins instead of sub-queries.

� Conflicts:
�	lib/Doctrine/ORM/Persisters/Collection/ManyToManyPersister.php
�	tests/Doctrine/Tests/ORM/Functional/EntityRepositoryTest.php
�	tests/Doctrine/Tests/ORM/Functional/Ticket/DDC192Test.php
�	tests/Doctrine/Tests/ORM/Functional/Ticket/DDC3634Test.php
�	tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6303Test.php
�	tests/Doctrine/Tests/ORM/Functional/xml/Doctrine.Tests.Models.OrnementalOrphanRemoval.Person.dcm.xml
�	tests/Doctrine/Tests/ORM/Functional/xml/Doctrine.Tests.Models.OrnementalOrphanRemoval.PhoneNumber.dcm.xml
�	tests/Doctrine/Tests/OrmFunctionalTestCase.php
�	tests/Doctrine/Tests/TestUtil.php
@FaritSlv FaritSlv requested review from beberlei, derrabus and greg0ire and removed request for beberlei, derrabus, greg0ire and morozov September 21, 2022 20:26
@FaritSlv
Copy link
Author

@greg0ire, I apologize, when resolve merge-conflicts I did not see the connection to oracle. Please run action

@greg0ire
Copy link
Member

Can you create a separate PR with a non-controversial commit? That way, once it is merged, you will not have to wait for us to click the "run action" button on this PR as you will be trusted.

@FaritSlv
Copy link
Author

Can you create a separate PR with a non-controversial commit? That way, once it is merged, you will not have to wait for us to click the "run action" button on this PR as you will be trusted.

Ок

@FaritSlv
Copy link
Author

Can you create a separate PR with a non-controversial commit? That way, once it is merged, you will not have to wait for us to click the "run action" button on this PR as you will be trusted.

Resolve
Static Analysis, Continuous Integration, Performance benchmark, Coding Standards

$offset = $criteria->getFirstResult();

return $this->platform->modifyLimitQuery('', $limit, $offset ?? 0);
if ($limit !== null || $offset !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary? Also note that with regards to $offset, zero and null should be treated equally. In fact, null should be deprecated by now.

Copy link
Author

Choose a reason for hiding this comment

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

Is it better to check if it is greater than zero?

Copy link
Member

Choose a reason for hiding this comment

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

No, it would be better to not change this line. I want to understand why you do.

Copy link
Member

Choose a reason for hiding this comment

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

And if we need this change, make sure that the values 0 and null for $offset create the same query. I was like this before your change.

[
'name' => 'phonenumbers.phonenumber',
'column' => 'number',
'column' => '`number`',
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename the column.

@FaritSlv
Copy link
Author

@derrabus, what else is required of me?

@derrabus
Copy link
Member

Sorry that this PR takes so long to get merged. The main issue is that the PR does not only add Oracle to the build matrix, it also changes a lot of code that at first glance is not specific to Oracle. This makes the changes hard to understand for us.

@FaritSlv
Copy link
Author

Sorry that this PR takes so long to get merged. The main issue is that the PR does not only add Oracle to the build matrix, it also changes a lot of code that at first glance is not specific to Oracle. This makes the changes hard to understand for us.

@derrabus, ok, i'm all understanding. Won't be surprised if they don't merge this PR, because very big

@derrabus
Copy link
Member

One thing you can do is to isolate changes and submit them as individual PRs. Smaller, focused PRs are usually easier to review and get merged more quickly.

@FaritSlv
Copy link
Author

One thing you can do is to isolate changes and submit them as individual PRs. Smaller, focused PRs are usually easier to review and get merged more quickly.

So the problem is that all the changes are related. I will think how to split

@greg0ire greg0ire removed their request for review June 22, 2023 20:44
@github-actions
Copy link
Contributor

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Nov 12, 2024
@github-actions
Copy link
Contributor

This pull request was closed due to inactivity.

@github-actions github-actions bot closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants