[
https://issues.apache.org/jira/browse/SLING-8854?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16982155#comment-16982155
]
Ashish Chopra commented on SLING-8854:
--------------------------------------
_making note of an off-JIRA discussion with [~marett] yesterday_
tl;dr:
[~marett] believes *it is OK to couple implementations of a
{{DistributionQueue}} and {{DistributionQueueProcessor}}*.
{{DistributionQueue}} impls should be dumb/stupid and shouldn't be made aware
of processing-attempts. It should be the responsibility of
{{DistributionQueueProcessor}} (aided by {{DistributionQueueProvider}})
implementations to track processing-attempts.
[~ashishc] believes that to maximise code-resuse and follow DRY, *it is _not_
OK to couple implementations of a {{DistributionQueue}} and
{{DistributionQueueProcessor}}*. It is totally up to the queue implementation
to decide _how_ (if at all) the processing-attempts should be recorded. To
achieve that _without_ coupling implementations of
{{DistributionQueueProcessor}} with those of {{DistributionQueue}} (and
{{DistributionQueueProvider}}), a mechanism for {{DistributionQueueProcessor}}
to communicate processing-attempts to the {{DistributionQueue}} is required -
this would mean updating {{DistributionQueue}} SPI and picking one of the two
alternatives [presented
here|https://issues.apache.org/jira/browse/SLING-8854?focusedCommentId=16981545&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16981545].
----
Tim's PoV:
{quote}
We should keep the implementation for recording processing attempts private,
unless there's a severe limitation that makes it impossible.
The fact that {{getHead}} increases the processing-attempt-count is a bug -
keeping the retry count in memory, or writing it in the repo is a definite
option, but it need not be exposed at the Queue SPI level. It can be kept
entirely within in implementation without exposing a method in the
{{DistributionQueue}} SPI.
The idea to let the queue count the number of processing-attempts (and persist
it) is totally against the principle of keeping the Queues dumb/stupid.
Taking the case of existing Sling Job based queue implementation as an
illustration, the {{.getHead}} invocation is side-effect free (i.e., no
behind-the-scenes increment of processing-attempts-counter). It is entirely up
to the Sling Queue implementation to record the counts whichever way it deems
fit.
For the "In-memory" and "In-File" based queues, perhaps the addition of a new
component could have achieved the same effect, but likely due to paucity of
time the {{.getHead}} also (incorrectly) updated the processing-attempt.
The suggestion here would be to make queue-implementations write its own
processor (or re-work the existing processor) _if_ they need such addition
capabilities (to account for processing-attempts) such that common parts and
specific parts are kept apart. Even when there are two parts to an
implementation, the interface between the two can be kept internal.
{quote}
Ashish's PoV:
{quote}
Processing-status of a queue-head can't be kept in-memory always. Queue
implementations might be interested in generating a cluster-consistent view of
processing-attempts made on a queue-head. Since persisting the
processing-attempt would involve working through the queue-implementation,
queue-impl is the most suitable place to record dequeue attempts.
On the intent of keeping the queues unaware of processing attempts, it is
noteworthy that {{DistributionQueue}} impls need to return a
{{DistributionQueueEntry}} via a {{.getEntry}} method.
{{DistributionQueueEntry}} encapsulates a {{DistributionQueueItemStatus}},
which has a method named {{.getAttempts}} to return the number of processing
attempts made against this {{DistributionQueueEntry}}.
Since {{DistributionQueue}} impls are _already_ expected to know about statuses
of their entries (processing-attempts is one of the items of the status), it
makes sense to let the {{DistributionQueue}} impls get an intimation about a
processing-attempt made by a {{DistributionQueueProcessor}} and dictate what to
do with it.
The trouble arises from the fact that (unlike typical queue interfaces),
{{DistributionQueue}} doesn't have a notion of distinct operations for "peek"
and "dequeue". Instead, it has a {{.getHead}} operation, which (based on the
intent of the caller) may not always map to "processing a queue entry". There
needs to be a notion of reading the head of the queue without incrementing the
attempt-count.
For Sling Job based queues, this happens by accident since the Sling engine
keeps the count of job-execution-attempts.
For everything else, the callers of {{.getHead}} must be able to indicate the
intent (i.e., do they intend to process the entry, or intend to simply
introspect the queue-head.
To enable that, we need a {{DistributionQueue.peek}}, which stuff like
Health-Check, MBeans, UI, etc. can call to get info about the queue-head
without calling {{.getHead}}, or we need
{{DistributionQueue.recordProcessingAttempt}}, which the
{{DistributionQueueProcessor}} impls can explicitly call, making the queue
aware of a processing-attempt _without_ expecting the
{{DistributionQueueProcessor}} to know the implementation details of a
{{DistributionQueue}}. Expecting that there should a 1:1 mapping between
{{DistributionQueue}} and {{DistributionQueueProcessor}}, and that
{{DistributionQueueProcessor}} should know about the implementation-details of
{{DistributionQueue}} severely limits the capabilities of the interfaces in
question.
{quote}
> In-file and In-memory queue-providers for Forward Distribution report
> incorrect value for processing "Attempts"
> ---------------------------------------------------------------------------------------------------------------
>
> Key: SLING-8854
> URL: https://issues.apache.org/jira/browse/SLING-8854
> Project: Sling
> Issue Type: Bug
> Components: Content Distribution
> Affects Versions: Content Distribution Core 0.4.0
> Reporter: Ashish Chopra
> Assignee: Timothee Maret
> Priority: Major
> Attachments:
> 0001-SLING-8854-In-file-and-In-memory-queue-providers-for.patch
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> {{ForwardDistributionAgentFactory}} allows specifying "In-Memory" and
> "In-File" queue-providers [0]. Both these implementations are provided by
> {{SimpleDistributionQueue}} [1].
> While responding to a queue-processing request, the head of the queue is
> fetched to create a {{DistributionQueueEntry}}, which encapsulates a
> {{DistributionQueueItemStatus}}. While retrieving the queue-head-entry, the
> number-of-attempts are also incremented by 1, implying _one more_
> processing-attempt to have happened on a given queue-item. [2]
> The trouble arises from the fact that {{SimpleDistributionQueue#getHead}}
> ends up being called from at least three other places:
> # Queue status Health Check [3], which is a periodic job
> # UI (via {{ExtendedDistributionServiceResourceProvider}}) [4]
> # Distribution Queue status MBean [5]
> Out of these, #1 is most troublesome - because of its periodic nature, the
> number of Attempts for a queue-item that's being processed (or, is blocked)
> will keep on increasing without corresponding dequeue-attempts.
> Due to #2, everytime the queue status is delivered to the UI, the attempt
> would again increase. Similar would be the case of MBean invocations.
> This needs to be addressed. Notably, "Sling Jobs" based queue doesn't suffer
> from this issue because it relies on retry-counts as maintained by Sling Job
> Execution Engine.
> [0]
> https://github.com/apache/sling-org-apache-sling-distribution-core/blob/deb3d2ae33c4f4678c8503091a9fffdbb141e569/src/main/java/org/apache/sling/distribution/agent/impl/ForwardDistributionAgentFactory.java#L258-L262
> [1]
> https://github.com/apache/sling-org-apache-sling-distribution-core/blob/deb3d2ae33c4f4678c8503091a9fffdbb141e569/src/main/java/org/apache/sling/distribution/queue/impl/simple/SimpleDistributionQueue.java
> [2]
> https://github.com/apache/sling-org-apache-sling-distribution-core/blob/deb3d2ae33c4f4678c8503091a9fffdbb141e569/src/main/java/org/apache/sling/distribution/queue/impl/simple/SimpleDistributionQueue.java#L110-L112
> [3]
> https://github.com/apache/sling-org-apache-sling-distribution-core/blob/deb3d2ae33c4f4678c8503091a9fffdbb141e569/src/main/java/org/apache/sling/distribution/monitor/DistributionQueueHealthCheck.java#L113
> [4]
> https://github.com/apache/sling-org-apache-sling-distribution-core/blob/deb3d2ae33c4f4678c8503091a9fffdbb141e569/src/main/java/org/apache/sling/distribution/resources/impl/ExtendedDistributionServiceResourceProvider.java#L158
> [5]
> https://github.com/apache/sling-org-apache-sling-distribution-core/blob/deb3d2ae33c4f4678c8503091a9fffdbb141e569/src/main/java/org/apache/sling/distribution/monitor/impl/DistributionQueueMBeanImpl.java#L62-L91
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
