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

Orion 15.2.0 psvamb 6815 #8852

Merged
merged 5 commits into from Nov 1, 2019
Merged

Orion 15.2.0 psvamb 6815 #8852

merged 5 commits into from Nov 1, 2019

Conversation

@hilak
Copy link
Contributor

hilak commented Nov 1, 2019

No description provided.

hilak added 5 commits Jul 11, 2019
New drop folder type
@hilak

This comment has been minimized.

Copy link
Contributor Author

hilak commented Nov 1, 2019

@hilak hilak merged commit b01d21f into Orion-15.10.0 Nov 1, 2019
2 checks passed
2 checks passed
Datree Smart Policy Best Practices Verification
Details
Datree insights datreeio insights events
Details
@hilak hilak deleted the Orion-15.2.0-PSVAMB-6815 branch Nov 1, 2019
Copy link
Contributor

MosheMaorKaltura left a comment

General code review remarks -

  1. Every if statement must have {} even if the execution is one line.
  2. String match must use ===
  3. Avoid using 'strings' and code and define CONST
  4. Add default in switch cases
{
if (!$baseEnumName)
{
return array('ApFeedDropFolderType');

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

Use const

{
return array('ApFeedDropFolderType');
}
if ($baseEnumName == 'DropFolderType')

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor
  1. Use === for string match
  2. For 'DropFolderType' use KalturaDropFolderType::getEnumClass()

This comment has been minimized.

Copy link
@hilak

hilak Nov 4, 2019

Author Contributor

@MosheMaorKaltura the reason this is done this way (in all of our plugins - not just this one) is because the IKalturaEnumerator::getEnums method is called from several endpoints - admin console, batch and generator. Each context has a different classmap loaded and in some cases this method is called in order to build the class map. In order to reference this value as a class enum, the class defining the enum needs to be part of the classmap, leading to an exception being thrown.

{
return new Kaltura_Client_ApFeedDropFolder_Type_ApFeedDropFolder();
}
break;

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

What should be the default behaviour of this switch case? should it print an message? throw exception do-nothing?
Please add default to the switch.

This comment has been minimized.

Copy link
@hilak

hilak Nov 4, 2019

Author Contributor

@MosheMaorKaltura there is no default on any of the plugin classes that implement the loadObject through a switch case. The reason is that this method is being called from the KalturaPluginManager, which iterates over all the plugins that implement the loadObject function. If all plugins had a default logging a message, you would see several KalturaLog messages all reading "Plugin xyz doesn't handle this class". If you threw an exception, you would prevent the PluginManager from processing the next plugin class which might be the correct one. Any logic added in a default case would be bad here so there is no default.

switch ($baseClass) {
case 'DropFolderFile':
if ($enumValue == self::getDropFolderTypeCoreValue(ApFeedDropFolderType::AP_FEED))
return 'FeedDropFolderFile';

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

Use const values

public function watchFolder(KalturaDropFolder $dropFolder)
{
/* @var $dropFolder KalturaApFeedDropFolder */
KalturaLog::info("Watching drop folder with ID [" . $dropFolder->id . "]");

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

No need to use double quotes here since there is no processing , please use single.

$counter = 0;
$break = false;
do{

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

Brackets in new line.

$feedUrl = $feed['data']['next_page'];
}
}while ($lastPageCount == self::AP_FEED_PAGE_SIZE);

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

This is a weak end case, since it is possible that due to changes that happen in parallel, one of the pages will return less than page size, still there will be more to come.
Safer approach will be until lastPageCount == 0.

This comment has been minimized.

Copy link
@hilak

hilak Nov 4, 2019

Author Contributor

@MosheMaorKaltura because this is a professionally generated news feed, odds are slim that we could catch it just at the time of day it gets updated, which is exactly why I cut off the interaction with it on what is perceived as the last page. Also I don't see how the same case can't happen even if the last page item count is 0. This interaction isn't a one-off - this is a drop folder pull, so it gets repeated periodically. Any content that was not ingested on a single run will likely get pulled in on the next one.

public function toObject($dbObject = null, $skip = array())
{
if (!$dbObject)
$dbObject = new ApFeedDropFolder();

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

add {}

$dbObject = new ApFeedDropFolder();
if ($this->feedItemInfo)
$dbObject->setFeedItemInfo($this->feedItemInfo->toObject());

This comment has been minimized.

Copy link
@MosheMaorKaltura

MosheMaorKaltura Nov 3, 2019

Contributor

add {}

/**
* @return the $apApiKey
*/
public function getApApiKey() {

This comment has been minimized.

Copy link
@MosheMaorKaltura
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.