softwaremill / elasticmq

In-memory message queue with an Amazon SQS-compatible interface. Runs stand-alone or embedded.
https://softwaremill.com/open-source/
Apache License 2.0
2.51k stars 194 forks source link

Queue creation order random #102

Closed slobo closed 7 years ago

slobo commented 7 years ago

As per #100, queues should not be created concurrently, instead they should either follow config file order, or in order that will satisfy deadletter queue dependencies. @adamw suggest problem is here: https://github.com/adamw/elasticmq/blob/release-0.13.2/server/src/main/scala/org/elasticmq/server/ElasticMQServer.scala#L63

Example failure:

conf:

queues {
    queue1 {
        deadLettersQueue {
            name = "queue1-deadletters"
            maxReceiveCount = 3 
        }
    }
    queue1-deadletters { }
}

log:

00:30:58.090 [main] INFO  org.elasticmq.server.Main$ - Starting ElasticMQ server (0.13.2) ...
00:30:58.378 [elasticmq-akka.actor.default-dispatcher-4] INFO  akka.event.slf4j.Slf4jLogger - Slf4jLogger started
00:30:58.382 [elasticmq-akka.actor.default-dispatcher-4] DEBUG akka.event.EventStream - logger log1-Slf4jLogger started
00:30:58.385 [elasticmq-akka.actor.default-dispatcher-4] DEBUG akka.event.EventStream - Default Loggers started
00:30:58.471 [elasticmq-akka.actor.default-dispatcher-3] DEBUG c.t.sslconfig.akka.AkkaSSLConfig - Initializing AkkaSSLConfig extension...
00:30:58.472 [elasticmq-akka.actor.default-dispatcher-3] DEBUG c.t.sslconfig.akka.AkkaSSLConfig - buildHostnameVerifier: created hostname verifier: com.typesafe.sslconfig.ssl.DefaultHostnameVerifier@6c61a903
00:30:59.003 [elasticmq-akka.actor.default-dispatcher-4] DEBUG akka.io.TcpListener - Successfully bound to /0:0:0:0:0:0:0:0:9324
00:30:59.007 [elasticmq-akka.actor.default-dispatcher-6] INFO  o.e.rest.sqs.TheSQSRestServerBuilder - Started SQS rest server, bind address 0.0.0.0:9324, visible server address * (depends on incoming request path) 
00:30:59.048 [elasticmq-akka.actor.default-dispatcher-6] DEBUG o.elasticmq.actor.QueueManagerActor - Cannot create queue, its dead letters queue doesnt exists: QueueData(queue1,MillisVisibilityTimeout(30000),PT0S,PT0S,2017-03-10T00:30:59.013Z,2017-03-10T00:30:59.013Z,Some(DeadLettersQueueData(queue1-deadletters,3)),None)
00:30:59.049 [elasticmq-akka.actor.default-dispatcher-6] INFO  o.elasticmq.actor.QueueManagerActor - Creating queue QueueData(queue1-deadletters,MillisVisibilityTimeout(30000),PT0S,PT0S,2017-03-10T00:30:59.039Z,2017-03-10T00:30:59.039Z,None,None)
mkubenka commented 7 years ago

There are 2 other problems except for concurrent creation of queues:

  1. ConfigObject doesn't keep entries order - typesafehub/config#365
  2. List used for createQueues in ElasticMQServerConfig doesn't keep order of items
ferrants commented 7 years ago

Instead of using ordering, we could just create any queues without deadletter queues and then do the ones with deadletter queues. Do we NEED to do them in order?

slobo commented 7 years ago

Can a dead-letter queue have own dead-letter queue? In that case we would need to calculate a dependancy graph.

In Perl land I've used Graph::topological_sort, I'm sure there is something similar for JVM?

(This of course assumes we don't want to allow any cycles - does that restriction exist in SQS?)

mkubenka commented 7 years ago

Following @ferrants suggestion, I have commented out exception thrown when dead letter queue doesn't exists. From quick test it looks like everything is working properly.

Changes: https://github.com/adamw/elasticmq/compare/master...mkubenka:fix/102-deadletter-queue-dependency

ferrants commented 7 years ago

@slobo i think a dlq can have another dlq, though I don't think it's a good move, architecturally speaking. can't think of any good cases for it. certainly cycles are not standard either.

@mkubenka , i think that not caring if a queue's dlq exists at the time of creation is a reasonable way to go. it'll push the error to when you're using the queues instead of at creation time. this would allow us to not concern ourselves with creation order. 👍

jononu commented 7 years ago

We use dead letter queues with another dead letter queue. Our primary queue polls N times, before dropping to an error dead letter queue where statuses get updated. It then drops to the next dead letter queue.

slobo commented 7 years ago

Yeah, i think there are a number of interesting things to do with chaining DLQs, i.e. you could implement an exponential back-off retry mechanism by stringing together a bunch of queues with different visibility timeouts.

Ideally, config file should be validated for any queue name misspellings, but if possible for ElasticMQ to just delay DLQ connection until the time it actually has to transfer into it, I think it's a good solution for now.

adamw commented 7 years ago

Fixed in 0.13.3