Add parameter to RepublishBuried signature to filter jobs #40

mcarmonaa · 2018-04-11T07:37:03Z

erizocosmico · 2018-04-11T07:51:01Z

queue/memory.go

+			if err := q.Publish(j); err != nil {
+				return err
+			}
+			// TODO: remove job fom q.buriedJobs


is this a leftover?

More or less. In fact I'm not sure, before that change it didn't remove the job from the slice too, so idk if is intended, a bug or it doesn't matter.

ajnavarro · 2018-04-11T08:45:20Z

queue/amqp.go

-
 		retries = 0
+		if comply(j) {
+			if err = j.Ack(); err != nil {


You should Ack the job after sending it to the other queue, and try to NACK if the publish fails.

I would also reject the jobs just after publishing error or failing compliance. Adding them to a slice to process afterwards can make some of them unprocessed if some error happens.

btw. did you think about dead-letter-exchange. we can use it for "non immediately retry".

@jfontan I can't reject immediately because I'd send back the job to the queue it's being iterated, so we will end up stuck in an infinite loop.

@kuba, this queue is already the dead-letter-exchange queue, so we would have to configure a buried queue for the buried queue, and republish from the child buried queue to the parent buried queue.

Finally we will have the same situation between those two buried queues. Not sure about it 😕

So maybe we need an another exchange with counter (and maybe size limit). If we exceed some q's size or timeout messages will be enqueued again but retry counter will be increased. If we got e.g. 5 retries and it's still failing we can drop a message. Maybe it's hard to explain as a comment but maybe just as a suggestion this is what I mean: https://felipeelias.github.io/rabbitmq/2016/02/22/rabbitmq-exponential-backoff.html
This is what me and my colleague (Felipe) implemented long time ago to avoid dropping messages and handle backoff.

ajnavarro · 2018-04-11T08:48:50Z

queue/amqp.go

-func (q *AMQPQueue) RepublishBuried() error {
+// RepublishBuried will republish in the main queue those jobs that timed out without Ack
+// or were Rejected with requeue = False and and makes complay return true.
+func (q *AMQPQueue) RepublishBuried(comply RepublishConditionFunc) error {


Maybe to do not break the API we should create a new method called RepublishBuriedConditionally with all that logic, and a RepublishBuried calling to RepublishBuriedConditionally with a function that accepts everything.

Or accept a ...RepublishConditionFunc which would allow several small validation functions and you can still call it as RepublishBuried() thus not breaking API compat

erizocosmico · 2018-04-12T13:45:27Z

queue/amqp.go

 	defer iter.Close()

 	retries := 0
+	notComplying := make([]*Job, 0, 3)


3 for any specific reason?

Nope, I just thought about having a defined small initial capacity.
Is there some recommendation for those cases?

just var something []Foo, it won't make much difference in terms of memory usage and/or speed and you don't have to worry about which number to pick

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

…arameter Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

jfontan · 2018-04-17T08:58:01Z

queue/amqp.go

-		if err = q.Publish(j); err != nil {
-			return err
+func handleRepublishErrors(list []*jobErr) error {
+	if len(list) > 1 {


Why 1? Shouldn't be > 0?

😳 Yes, it should be!

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa · 2018-04-18T15:26:36Z

ping @ajnavarro

ajnavarro

Small minor docs changes and LGTM

ajnavarro · 2018-04-20T07:30:47Z

queue/amqp.go

+}
+
+// RepublishBuried will republish in the main queue those jobs that timed out without Ack
+// or were Rejected with requeue = False and and makes complay return true.


Reword this please; remove duplicated and and complay => comply

ajnavarro

Also, we should add some tests

mcarmonaa · 2018-04-20T13:13:00Z

@ajnavarro I already added a test and, incidentally, caught a 🐛

jfontan · 2018-04-20T13:38:49Z

queue/common.go

+
+type republishConditions []RepublishConditionFunc
+
+func (c republishConditions) comply(job *Job) bool {


Couldn't this be simplified?

if len(c) == 0 { return true } for _, condition := range c { if condition(job) { return true } } return false

Otherwise LGTM

yes, it looks like much better 👍

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa mentioned this pull request Apr 11, 2018

[WIP] Add logic to the producer to be able to requeue failed jobs src-d/borges#250

Closed

mcarmonaa requested review from ajnavarro, erizocosmico, jfontan and kuba-- April 11, 2018 07:37

erizocosmico reviewed Apr 11, 2018

View reviewed changes

ajnavarro suggested changes Apr 11, 2018

View reviewed changes

erizocosmico reviewed Apr 12, 2018

View reviewed changes

mcarmonaa added 3 commits April 13, 2018 08:04

Add paramanter to RepublishBuried signature to filter jobs

8186142

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

Handle republish errors

4219edd

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

Change RepublishBuried to accept '...RepublishConditionFunc' as the p…

2b23461

…arameter Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa force-pushed the improvement/queue-republish-buried-filter branch from 13d2dbe to 2b23461 Compare April 13, 2018 06:06

mcarmonaa mentioned this pull request Apr 13, 2018

producer republish command src-d/borges#253

Merged

jfontan reviewed Apr 17, 2018

View reviewed changes

fix handleRepublishErrors bug in len(list) checking

ffea2b3

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa changed the title ~~Add paramanter to RepublishBuried signature to filter jobs~~ Add parameter to RepublishBuried signature to filter jobs Apr 18, 2018

ajnavarro approved these changes Apr 20, 2018

View reviewed changes

ajnavarro reviewed Apr 20, 2018

View reviewed changes

erizocosmico approved these changes Apr 20, 2018

View reviewed changes

jfontan approved these changes Apr 20, 2018

View reviewed changes

mcarmonaa added 2 commits April 20, 2018 15:49

queue: fix republish bug

886c2b0

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

queue: add test for amqp RepublishBuried

e5c6eaf

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>

mcarmonaa force-pushed the improvement/queue-republish-buried-filter branch from 8c743a1 to e5c6eaf Compare April 20, 2018 13:49

mcarmonaa merged commit 88f5399 into src-d:master Apr 20, 2018

mcarmonaa deleted the improvement/queue-republish-buried-filter branch April 20, 2018 14:29


		type republishConditions []RepublishConditionFunc

		func (c republishConditions) comply(job *Job) bool {

Add parameter to RepublishBuried signature to filter jobs #40

Add parameter to RepublishBuried signature to filter jobs #40

Uh oh!

Conversation

mcarmonaa commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarmonaa Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarmonaa commented Apr 18, 2018

Uh oh!

ajnavarro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajnavarro left a comment

Choose a reason for hiding this comment

Uh oh!

mcarmonaa commented Apr 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mcarmonaa Apr 12, 2018 •

edited

Loading