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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a38ee5f
No-Plat: Source map will not return by default + serve latest bundle …
yossipapi Oct 20, 2024
bb50013
Remove temp debug line
yossipapi Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 101 additions & 15 deletions alpha/apps/kaltura/modules/extwidget/actions/embedPlaykitJsAction.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class embedPlaykitJsAction extends sfAction
const REGENERATE_PARAM_NAME = "regenerate";
const IFRAME_EMBED_PARAM_NAME = "iframeembed";
const AUTO_EMBED_PARAM_NAME = "autoembed";
const INCLUDE_SOURCE_MAP_PARAM_NAME = 'includeSourceMap';
const LATEST = "{latest}";
const BETA = "{beta}";
const CANARY = "{canary}";
Expand Down Expand Up @@ -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.

private $uiConfTags = array(self::PLAYER_V3_VERSIONS_TAG);

public function execute()
Expand All @@ -56,16 +58,41 @@ public function execute()

$this->initMembers();

$updateUiConfBundleCacheKey = true;
$bundleContent = $this->bundleCache->get($this->bundle_name);
$i18nContent = $this->bundleCache->get($this->bundle_i18n_name);
$extraModulesNames = unserialize($this->bundleCache->get($this->bundle_extra_modules_names));

if (!$bundleContent || $this->regenerate)
{
list($bundleContent, $i18nContent, $extraModulesNames) = kLock::runLocked($this->bundle_name, array("embedPlaykitJsAction", "buildBundleLocked"), array($this), 2, 30);
try
{
list($bundleContent, $i18nContent, $extraModulesNames, $updateUiConfBundleCacheKey) = kLock::runLocked($this->bundle_name, array("embedPlaykitJsAction", "buildBundleLocked"), array($this), 2, 30);
}
catch (kCoreException $ex)
{
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.

list($bundleContent, $i18nContent, $extraModulesNames, $updateUiConfBundleCacheKey) = $this->tryServingExistingCacheVersion($this, KExternalErrors::BUNDLE_CREATION_FAILED, "Failed to build bundle in allocated time");
if(!$bundleContent)
{
KExternalErrors::dieError(KExternalErrors::BUNDLE_CREATION_FAILED, "Failed to serve bundle content");
}
break;
default:
KExternalErrors::dieError(KExternalErrors::BUNDLE_CREATION_FAILED, "Failed to serve bundle content with unknown error");
}
}
}

$lastModified = $this->getLastModified($bundleContent);

//Try saving bundle content cache key if needed for serving old version in case of an error while generating new one
if($updateUiConfBundleCacheKey)
{
$this->saveCurrentBundleCacheKey($this);
}

//Format bundle content
$bundleContent = $this->formatBundleContent($bundleContent, $i18nContent, $extraModulesNames);
Expand All @@ -88,7 +115,7 @@ public static function buildBundleLocked($context)
{
$i18nContent = $context->bundleCache->get($context->bundle_i18n_name);
$extraModulesNames = unserialize($context->bundleCache->get($context->bundle_extra_modules_names));
return array($bundleContent, $i18nContent, $extraModulesNames);
return array($bundleContent, $i18nContent, $extraModulesNames, false);
}
}

Expand All @@ -99,25 +126,31 @@ public static function buildBundleLocked($context)
KExternalErrors::dieError(KExternalErrors::BUNDLE_CREATION_FAILED, $config . " wrong config object");
}

$url = $context->bundlerUrl . "/build?config=" . base64_encode($config) . "&name=" . $context->bundle_name . "&source=" . base64_encode($context->sourcesPath);
$url = $context->bundlerUrl . "/build?config=" . base64_encode($config) . "&name=" . $context->bundle_name . "&source=" . base64_encode($context->sourcesPath) . "&includeSourceMap=" . $context->includeSourceMap;
$content = KCurlWrapper::getContent($url, array('Content-Type: application/json'), true);

if (!$content)
{
KExternalErrors::dieError(KExternalErrors::BUNDLE_CREATION_FAILED, $config . " failed to get content from bundle builder");
return $context->tryServingExistingCacheVersion($context, KExternalErrors::BUNDLE_CREATION_FAILED, $config . " failed to get content from bundle builder");
}

$content = json_decode($content, true);
if (isset($content['status'])) {
if ($content['status'] != 0) {
if (isset($content['status']))
{
if ($content['status'] != 0)
{
$message = $content['message'];
KExternalErrors::dieError(KExternalErrors::BUNDLE_CREATION_FAILED, $config . ". " . $message);
} else {
return $context->tryServingExistingCacheVersion($context, KExternalErrors::BUNDLE_CREATION_FAILED, $config . ". " . $message);
}
else
{
$content = $content['payload'];
}
} else {
if (!$content || !$content['bundle']) {
KExternalErrors::dieError(KExternalErrors::BUNDLE_CREATION_FAILED, $config . " bundle created with wrong content");
}
else
{
if (!$content || !$content['bundle'])
{
return $context->tryServingExistingCacheVersion($context, KExternalErrors::BUNDLE_CREATION_FAILED, $config . " bundle created with wrong content");
}
}

Expand All @@ -137,8 +170,8 @@ public static function buildBundleLocked($context)
{
KalturaLog::log("Error - failed to save bundle content in cache for config [".$config."]");
}

return array($bundleContent, $i18nContent, $extraModulesNames);
return array($bundleContent, $i18nContent, $extraModulesNames, true);
}

private static function getExtraModuleNames($extraModules = array())
Expand Down Expand Up @@ -777,8 +810,11 @@ private function initMembers()
if (!$this->partner)
KExternalErrors::dieError(KExternalErrors::PARTNER_NOT_FOUND);

//Get should force regenration
//Get should force regeneration
$this->regenerate = $this->getRequestParameter(self::REGENERATE_PARAM_NAME);

//Should we include player source map in the request result
$this->includeSourceMap = $this->getRequestParameter(self::INCLUDE_SOURCE_MAP_PARAM_NAME, "false");

//Get the list of partner 0 uiconf tags for uiconfs that contain {latest} and {beta} lists
$embedPlaykitConf = kConf::getMap(kConfMapNames::EMBED_PLAYKIT);
Expand Down Expand Up @@ -865,4 +901,54 @@ public function getRequestParameter($name, $default = null)
$returnValue = parent::getRequestParameter($name, $default);
return $returnValue ? $returnValue : $default;
}

protected function saveCurrentBundleCacheKey($context)
{
//Avoid updating current cache hash on the ui conf if dynamic params affecting the result are being sent
$versions = $this->getRequestParameter(self::VERSIONS_PARAM_NAME);
if($versions)
Comment on lines +908 to +909
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.

{
return;
}

if($context->bundle_name != $context->uiConf->getCurrentCacheKey())
{
//Avoid system load by controlling the percentage of ui confs how will get this update
if(rand(0, 100) < kConf::get("current_version_save_ration", kConfMapNames::EMBED_PLAYKIT, 30))
{
$context->uiConf->setCurrentCacheKey($context->bundle_name);
$context->uiConf->save();
}
else
{
KalturaLog::debug("Skipping ui conf update to avoid unnecessary system load");
}
}
}

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 •
edited
Loading

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.

KalturaLog::debug("TTT: existingCacheVersion [$existingCacheVersion]");
yossipapi marked this conversation as resolved.
Show resolved Hide resolved
if(!$existingCacheVersion)
{
KExternalErrors::dieError($errCode, $message);
}

$i18nContent = null;
$extraModulesNames = null;
$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 '

$extraModulesNames = unserialize($context->bundleCache->get($existingCacheVersion . "_extramodules"));
}

if(!$bundleContent)
{
KExternalErrors::dieError($errCode, $message);
}

return array($bundleContent, $i18nContent, $extraModulesNames, false);
}
}
12 changes: 11 additions & 1 deletion alpha/lib/model/uiConf.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ class uiConf extends BaseuiConf implements ISyncableFile, IRelatedObject

const CUSTOM_DATA_CON_FILE_VERSION = 'conf_file_version';
const CUSTOM_DATA_CONF_FILE_FEATURES_VERSION = 'conf_file_features_version';

const CUSTOM_DATA_V2REDIRECT = "v2redirect";
const CUSTOM_DATA_CURRENT_CACHE_KEY = 'current_cache_key';

public function save(PropelPDO $con = null)
{
Expand Down Expand Up @@ -827,4 +827,14 @@ public function setV2Redirect($v)
KalturaLog::log(JSON_encode($v));
return $this->putInCustomData(self::CUSTOM_DATA_V2REDIRECT, $v);
}

public function getCurrentCacheKey()
{
return $this->getFromCustomData( self::CUSTOM_DATA_CURRENT_CACHE_KEY);
}

public function setCurrentCacheKey($v)
{
return $this->putInCustomData(self::CUSTOM_DATA_CURRENT_CACHE_KEY, $v);
}
}