MCLOUD-6139 - MCLOUD-6211: Redis improvement - Create patches for 2.3.0 - 2.3.5

[BaDos]

Description

Added patches that contain performance improvements for Redis from Magento 2.3.5 and 2.3-develop branch.

Fixed Issues (if relevant)

1. https://jira.corp.magento.com/browse/MCLOUD-6139
2. https://jira.corp.magento.com/browse/MCLOUD-6211

Manual testing scenarios

For Magento 2.3.0 and 2.3.1 need to smoke testing product functionality.
There are steps for testing for Magento 2.3.0 - 2.3.5

1. Edit composer.json

"repositories": {
        "repo2": {
            "type": "composer",
            "url": "https://repo.magento.com/"
        },
        "ece-tools": {
            "type": "vcs",
            "url": "https://github.com/magento/ece-tools.git"
        },
        "mcp": {
            "type": "vcs",
            "url": "git@github.com:magento/magento-cloud-patches.git"
        },
        "docker":{
            "type": "vcs",
            "url": "git@github.com:magento/magento-cloud-docker.git"
        }
  },

"require": {
        "magento/magento-cloud-metapackage": ">=2.3.3 <2.3.4",
        "magento/magento-cloud-patches": "dev-MCLOUD-6139 as 1.0.99",
        "magento/ece-tools": "dev-MCLOUD-6327 as 2002.1.99"
},

2. Run composer update
3. Commit changes of composer.json and composer.lock
4. Push to trigger redeploy
5. Check that frontend works
6. Check app/etc/env.php

'cache' =>
  array (
    'frontend' =>
    array (
      'default' =>
      array (
        'backend' => 'Cm_Cache_Backend_Redis',
        'backend_options' =>
        array (
          'server' => 'redis.internal',
          'port' => 6379,
          'database' => 1,
        ),
        'id_prefix' => 'f53_',
      ),
      'page_cache' =>
      array (
        'backend' => 'Cm_Cache_Backend_Redis',
        'backend_options' =>
        array (
          'server' => 'redis.internal',
          'port' => 6379,
          'database' => 2,
        ),
        'id_prefix' => 'f53_',
      ),
    ),
  ),

7. Add .magento.env.yaml

stage:
  deploy:
    CACHE_CONFIGURATION:
      _merge: false
      frontend:
        default:
          backend: '\Magento\Framework\Cache\Backend\RemoteSynchronizedCache'
          backend_options:
            remote_backend: '\Magento\Framework\Cache\Backend\Redis'
            remote_backend_options:
              persistent: 0
              server: 'redis.internal'
              database: '1'
              port: '6379'
              password: ''
              compress_data: '1'
            local_backend: 'Cm_Cache_Backend_File'
            local_backend_options:
              cache_dir: '/dev/shm/'
          frontend_options:
            write_control: false
      type:
        default:
          frontend: 'default'

8. Commit .magento.env.yaml
9. Push to trigger redeploy
10. Check that frontend works
11. Check app/etc/env.php

'cache' =>
  array (
    'frontend' =>
    array (
      'default' =>
      array (
        'backend' => '\\Magento\\Framework\\Cache\\Backend\\RemoteSynchronizedCache',
        'backend_options' =>
        array (
          'remote_backend' => '\\Magento\\Framework\\Cache\\Backend\\Redis',
          'remote_backend_options' =>
          array (
            'persistent' => 0,
            'server' => 'redis.internal',
            'database' => '1',
            'port' => '6379',
            'password' => '',
            'compress_data' => '1',
          ),
          'local_backend' => 'Cm_Cache_Backend_File',
          'local_backend_options' =>
          array (
            'cache_dir' => '/dev/shm/',
          ),
        ),
        'frontend_options' =>
        array (
          'write_control' => false,
        ),
      ),
    ),
    'type' =>
    array (
      'default' =>
      array (
        'frontend' => 'default',
      ),
    ),
  ),

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[BarnyShergold]

PR description needs full information - a ticket link is not sufficient as many reviewers do not have access to the the JIRA tickets

[BarnyShergold]

If this is a "Create Patch" PR - why are these being removed? Are the Redis patches replacing these?

[BaDos]

The patches in this PR contain the same improvements and more.

[BarnyShergold]

Conditional will do -
$result = !$this->tryToLock($fileResource);

[BarnyShergold]

Should be a constant as it never changes

[BarnyShergold]

Should be a constant as it never changes

[BarnyShergold]

Should be a constant as it never changes

[BarnyShergold]

Test for array

[BarnyShergold]

Remove whitespace

[BarnyShergold]

WHy has this been removed?

[BarnyShergold]

WHy has this been removed?

[BarnyShergold]

Pure text should be a constant

[BaDos]

Hi @BarnyShergold
Thank you for your comments, but we do not change code in patches. This code from Magento Core.
If we change something it will be difficult to support patches in the future.
Sometimes support team provides some hotfixes to customers and if we change some part of code in patches it can be conflicts between patches.

Patches contain the same code as Magento Core.

[BarnyShergold]

@BaDos - I've only commented on NEW code added by the patches - this should surely be subject to the same code rules as standard code? It's why i've not commented on code that should be refactored - it's core.

I spent 3hrs going through all 10 files.

[BaDos]

@BarnyShergold, these are diffs from core Magento, where this code was fixed already. To be able to change the content of patches we would need to change the same in core first. If there are so many changes required though, I assume this is something that may need to be looked at by core team, since the patches used here are taken from latest core code.

[BarnyShergold]

@BaDos - AH I see how these patches are being created now.... Seems I wasted some of my own time - however the comments should still stand, so is there a mechanism to have these reported against the CORE code?

You've corrected the pull request description so can you tell me then - when these PRs for patches are submitted, what exactly are we reviewers reviewing?

If you can answer that - then I'll be happy to approve the PR and put it down to "lesson learned" 😄

[billygilbert]

@BaDos - AH I see how these patches are being created now.... Seems I wasted some of my own time - however the comments should still stand, so is there a mechanism to have these reported against the CORE code?

You've corrected the pull request description so can you tell me then - when these PRs for patches are submitted, what exactly are we reviewers reviewing?

If you can answer that - then I'll be happy to approve the PR and put it down to "lesson learned" 😄

@BarnyShergold When I review these, I am usually checking for:

1. Version constraints look proper
2. Modules that the patch is checking are appropriate
3. Patch construction is appropriate (pathing, headers for the patch, etc.)

[BarnyShergold]

Based on the CORRECT way to review patches, the only query I have that remains is the one for patches.json

[BarnyShergold]

If this is now 2.3.0 - 2.3.5 - This should be ">=2.3.2 <2.3.6" ?

[BaDos]

This patch MCLOUD_6139__improvement_flock_locks__2.3.2.patch contains changes for Magento 2.3.2 - 2.3.4-px where x is some number. So, we cannot use a constraint like this 2.3.2 - 2.3.4 and we use this one >=2.3.2 <2.3.5

[BarnyShergold]

Okay that's fine - just wanted to check that was the reason!

[mveeramneni]

QA Approved