MH-13244 Improve concurrency of OAIPMH republication #581
Conversation
KatrinIhler
force-pushed
the
t/MH-13244-oaipmh-republication-concurrency-problem
branch
from
33a90ea
to
b1b158a
Compare
|
Hi @KatrinIhler I've just had a look at this pull request. Here some comments:
Anyway, looking forwards to see this in production ;-) |
From what it seems like, there will not even be another 4.x release. 4.x EOL is in four days and no release is planned at the moment. Re ACL: I'll take a look at it… |
|
@lkiesow What I mean is that if the series ACL and the event ACL are to be both considered (ACL merging), you certainly don't want the episode ACL to be removed when updating the series ACL. |
|
@staubesv, the |
KatrinIhler
force-pushed
the
t/MH-13244-oaipmh-republication-concurrency-problem
branch
from
b1b158a
to
34e2b42
Compare
|
@staubesv Rebased the PR, ready for review again! |
| @@ -882,11 +883,13 @@ protected MetadataList deserializeMetadataList(String json) throws ParseExceptio | |||
| @Produces({ "application/json", "application/v1.0.0+json" }) | |||
| @RestQuery(name = "updateseriesacl", description = "Updates a series' access policy.", returnDescription = "", pathParameters = { | |||
| @RestParameter(name = "seriesId", description = "The series id", isRequired = true, type = STRING) }, restParameters = { | |||
| @RestParameter(name = "acl", isRequired = true, description = "Access policy", type = STRING) }, reponses = { | |||
| @RestParameter(name = "acl", isRequired = true, description = "Access policy", type = STRING), | |||
| @RestParameter(name = "override", isRequired = false, defaultValue = "false", description = "If true the series ACL will take precedence over any existing episode ACL", type = STRING)}, reponses = { | |||
There was a problem hiding this comment.
I hate to say it, but this is a change to the External API which we should not do without introducing a new minor version of the External API.
Can you somehow split this out of this PR and ensure that the External API will still behave the same as before?
There was a problem hiding this comment.
Actually I don't think this changes anything - if the override parameter isn't used here, it defaults to false, which is the same behavior as before since the override happened not in the series service but in the ACLService which isn't used by the External API.
There was a problem hiding this comment.
It does introduce the form parameter "override" to the request PUT /api/series/{series_id}/acl which is an API change. Without introducing a new version of the External API, there would then be different versions of 1.1.0, for example: One 1.1.0 that does not support this parameter, and one 1.1.0 that does.
But that implies that a client cannot determine whether the parameter override is supported based on the version of the External API as returned by /api/versions.
Besides that, you should update the specificaiton of the External API (https://docs.opencast.org/develop/developer/api/series-api/#put-apiseriesseries_idacl).
I suggest to remove the new argument and just default "override" to whatever makes it behave the same as before.
Later, we can introduce the parameter "override" in version 1.2.0 of the External API. For now, a JIRA issue ("Add parameter override to PUT /api/series/{series_id}/acl") should do.
There was a problem hiding this comment.
You're right, I'm gonna update this PR accordingly.
KatrinIhler
force-pushed
the
t/MH-13244-oaipmh-republication-concurrency-problem
branch
from
34e2b42
to
ed7c045
Compare
Remove the snapshot after removing the episode ACL so that the likelyhood that two OAIPMH distribution jobs run concurrently for the same mediapackage (which can result in a lost update) is minimized.
KatrinIhler
force-pushed
the
t/MH-13244-oaipmh-republication-concurrency-problem
branch
from
ed7c045
to
ac37bfa
Compare
|
@staubesv I removed the part affecting the external API. Once this is merged I can create a separate PR for develop that reintroduces this (or is there a fixed point in time when the External API is updated?) |
|
@KatrinIhler Thanks! There is not really a fixed point for the introduction of new minor versions of the External API. As a minor version implies added functionality, the only restriction is that the patch needs to go to develop. While a new version is in progress on develop, everybody else can take the chance to add backwards-compatible changes to that version if required. |
Split apart from PR #581.
Split apart from PR #581.
| @@ -51,6 +51,7 @@ | |||
| private final Boolean optOut; | |||
| private final String element; | |||
| private final String elementType; | |||
| private final Boolean overrideEpisodeAcl; | |||
There was a problem hiding this comment.
This, unfortunately, broke sending any series related messages in develop.
The issue is fixed by pull request #659.
Please avoid using any Objects except Strings in these message items.
There was a problem hiding this comment.
Thanks for noticing, I'll try to keep this in mind for the future.
KatrinIhler
deleted the
t/MH-13244-oaipmh-republication-concurrency-problem
branch
Split apart from PR #581.
Split apart from PR #581.
Split apart from PR #581.
Split apart from PR #581.
Split apart from PR #581.
Split apart from PR #581.
Split apart from PR #581.
Split apart from PR opencast#581.
Remove the snapshot after removing the episode ACL and move the whole step to the AssetManagerUpdatedEventHandler so that the likelihood that two OAIPMH distribution jobs run concurrently for the same mediapackage (which can result in a lost update) is minimized. (See MH-13233.)