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

No-Plat: Source map will not return by default + serve latest bundle … #12937

Open
wants to merge 2 commits into
base: Ursa-21.1.0
Choose a base branch
from

Conversation

yossipapi
Copy link
Collaborator

…if failed to create new one

{
switch ($ex->getCode())
{
case kCoreException::LOCK_TIMED_OUT:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this case cover the 2 cases? case 1 - Bundle is in action and new requests are rejected - 5xx error,
case 2 - Bundler failed / reset and returns 4xx error?

The only case that should return error is when the bundled content is missing some of the pieces and user gets explicit error in the response

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This covers only lock timeout the other changes in the code that call tryServingExistingCacheVersion are the cases that handle bundle failure.

$bundleContent = $context->bundleCache->get($existingCacheVersion);
if ($bundleContent)
{
$i18nContent = $context->bundleCache->get($existingCacheVersion . "_i18n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

use '

@@ -47,6 +48,7 @@ class embedPlaykitJsAction extends sfAction
private $playerConfig = null;
private $uiConfUpdatedAt = null;
private $regenerate = false;
private $includeSourceMap = "false";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest to load this default value from config, so in case we are catastrophy mode, we can change this value fast, but in general - hold it with value true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't make sense the default will be true as the entire purpose of this change is that the source map won't be returned by default.

Comment on lines +908 to +909
$versions = $this->getRequestParameter(self::VERSIONS_PARAM_NAME);
if($versions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most KMS related calls are using versions, so not sure why to remove this, assume that all the case coming from a website to load UI conf, may constantly come with versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As the list of versions sent by KMS is dynamic and can change I want to avoid constantly changing the hash values associated with the player. When sending versions this means there is no constant bundle hash and this is a use case I don't wont to handle.


protected function tryServingExistingCacheVersion($context, $errCode, $message = null)
{
$existingCacheVersion = $context->uiConf->getCurrentCacheKey();
Copy link
Collaborator

Choose a reason for hiding this comment

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

general note -
Saving current cache key on UI conf is not essential, as the current cache key can be calculated 'on the spot' from the versions that are currently required by it ( hash( ui confi versions ) while the action of saving new version on UICONF in DB, will create load on DB any time new version will be released.

Copy link
Collaborator Author

@yossipapi yossipapi Oct 22, 2024

Choose a reason for hiding this comment

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

The entire point is that the current hash of versions is not in the cache, this is why we got to this point in the first place. I need the old hash of the UI conf versions, which I currently don't have a way to get. If in the future we will have the old version map also stored we can re-visit this.

Also, this is why I added "if(rand(0, 100) < kConf::get("current_version_save_ration", kConfMapNames::EMBED_PLAYKIT, 30))" which means the save will be random with a low percentage so we won't cause any system load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants