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

Clarify text in the Messaging/Notification section #106

Merged
merged 1 commit into from May 26, 2017

Conversation

@acoburn
Copy link
Contributor

commented Apr 21, 2017

No description provided.

@barmintor barmintor requested review from awoods, escowles and dannylamb May 10, 2017
@barmintor

This comment has been minimized.

Copy link
Contributor

commented May 10, 2017

@dannylamb this seems to be mostly nomenclature, and not a decline in specification. Thoughts from the Islandora perspective? @escowles ditto Hydra?

@ajs6f
ajs6f approved these changes May 10, 2017
Copy link
Contributor

left a comment

I'm not sure how strongly asynchrony is implied by "messaging", but I agree that "notification" implies it even less.

Copy link
Contributor

left a comment

Most Hydra repositories do not use messaging now, so from the Hydra perspective, this change is unlikely to impact us one way or the other.

I agree with @barmintor that this looks like a nomenclature change, and the requirements for when notifications are generated and what they contain look equivalent, and the text is considerably streamlined too.

@awoods

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2017

I will give this a look later today / this evening.

@zimeon

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

General sense seems fine although I think it would be nice to keep the following two clarifications:

A failed operation (HTTP 4xx or 5xx) MUST NOT emit any messages.

Messages MUST NOT be emitted until after any changes have been persisted to durable storage.

I assume these are taken as implied still, but I prefer making them explicit.

@ajs6f

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

A failed operation (HTTP 4xx or 5xx) MUST NOT emit any messages.

I would prefer to let this go out. An impl might (for audit purposes or similar) prefer to offer the option of (e.g.) notification for 403 or the like.

@zimeon

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

It seems from comments above that the intention here is to change scope quite significantly (somewhat beyond agreements in #105 and #102), which perhaps needs some discussion (and maybe use cases).

I'm fine with dropping persisted to durable storage down to persisted (in whatever notion of persisted a particular implementation has). However, if we want to allow a much broader scope of events that generate notifications we should say something about that. As currently written the text is all about emitting notifications for resource changes. If there is agreement to extend that to other conditions then there should be changes along the lines of

For every resource whose state is changed as a result of an HTTP operation, there MUST be a corresponding notification made available describing that change.

to

For every resource whose state is changed as a result of an HTTP operation, there MUST be a corresponding notification made available describing that change. Implementations MAY also emit notifications for other operations.

@ajs6f

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

@zimeon I would be even more generic: "Implementations MAY also emit notifications for other reasons."

</p>
<p>
Repository start, stop and configuration change operations MAY cause a message to be emitted.
Implementers should be aware that some operations cause multiple resources to change.
In these cases, there will be a corresponding notification describing each of the changes.

This comment has been minimized.

Copy link
@awoods

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 12, 2017

Contributor

I understood @awoods 's comment to be referring to the question of ordering-- that the spec should prevent inferring the ordering of operations from the ordering of notifications. Now I think I must have misunderstood the comment.

This comment has been minimized.

Copy link
@barmintor

barmintor May 12, 2017

Contributor

I'm sympathetic to @awoods, it's difficult to track the requirement deltas in a text diff (but @acoburn's PR is more legible IMO, and that's the cost) . Here's the removals that I see going over it trying to pull all the norm indicators out:

  • "Repository start, stop and configuration change operations MAY cause a noun to be emitted."
  • "nouns MAY contain a timestamp marking the time the resource was added/modified/deleted."
  • "A failed operation (HTTP 4xx or 5xx) MUST NOT emit any nouns."
  • "nouns MUST NOT be emitted until after any changes have been persisted to durable storage."
  • Emitted events MUST contain exactly one value (cardinality removed) for resource identifier/IRI
  • Emitted events MUST contain exactly one value for event identifier

I don't think there are any new requirements in this PR, tho some requirements individually explicated have been collapsed. @acoburn does that line up with your intentions? I think the removal of the event identifier is the only one that surprises me, but because it's in a consolidated set of requirements it may have been an oversight? I can't remember if that was discussed.

This comment has been minimized.

Copy link
@ajs6f

ajs6f May 12, 2017

Contributor

I have no intention of impling the "atomic ops" spec, but I can imagine impling WebDAV's MOVE HTTP message to meet the same use cases. For that message, it would be natural to emit a 2-resource-IRI notification.

This comment has been minimized.

Copy link
@barmintor

barmintor May 12, 2017

Contributor

So the language here is around messages changing to notifications, but there is another noun here- event. If a notification can be a collection, is it advisable to still recommend single resource event descriptions within the notification? That seems to be the missing link.

This comment has been minimized.

Copy link
@barmintor

barmintor May 12, 2017

Contributor

@acoburn I understand that with regard to the entireity of the notification, I only want to make sure that there's not a reconciliation with the motivation behind the deleted text at the level of event description, which seems like a second entity.

A message SHOULD contain at least one value for the agent operating on the resource.
</p>
<p>
A message SHOULD contain a collection of RDF types corresponding to the resource that

This comment has been minimized.

Copy link
@awoods

awoods May 12, 2017

Collaborator

Since we are deferring to the activitystreams spec, I am not seeing MUST language for event types (or activity types). Are you intentionally dropping this? If not, can you point to the reference in the activitystreams spec that specifies it msut be included?

This comment has been minimized.

Copy link
@awoods

awoods May 12, 2017

Collaborator

Thanks for pointing the relevant diff out.

@awoods
awoods approved these changes May 26, 2017
@zimeon
zimeon approved these changes May 26, 2017
@awoods awoods merged commit 2cb59c9 into fcrepo:master May 26, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@acoburn acoburn deleted the acoburn:clarify_messaging branch May 26, 2017
This was referenced May 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.