roq-messaging / RoQ

RoQ, Elastically scalable MOM
21 stars 4 forks source link

Improper handling of duplicate AS rule name #91

Closed jbruggem closed 11 years ago

jbruggem commented 12 years ago

When inserting a AS rule using the name of an already existing rule, the insertion is reported as successful to the BSON API client (it should be reported as failed).

Moreover, there's an unhandled SQL exception in the GCM's log.

DEBUG [Thread-2] (MngtController.java:365) - ADD autoscaling rule request received ...
DEBUG [Thread-2] (MngtController.java:373) - Adding autoscaling configuration for test1
DEBUG [Thread-2] (MngtServerStorage.java:523) - Getting Q test1: 172.23.78.146 false AS config  0
DEBUG [Thread-2] (MngtController.java:397) - Host scaling rule decoded : Host scaling rule id = 0 [cpu:40, ram:30]
 INFO [Thread-2] (AutoScalingRuleStorageManager.java:261) - Inserting 1 new Physical host Auto scaling  configuration: Host scaling rule id = 0 [cpu:40, ram:30]
DEBUG [Thread-2] (MngtController.java:412) - Xchange scaling rule : Host scaling rule id = 3 [cpu:40, ram:30]
 INFO [Thread-2] (AutoScalingRuleStorageManager.java:176) - Inserting 1 new Exchange Auto scaling  configuration: Exchange scaling rule  id = 0 [0, 20000, 0.0]
DEBUG [Thread-2] (MngtController.java:427) - Host scaling rule : Host scaling rule id = 3 [cpu:40, ram:30]
 INFO [Thread-2] (AutoScalingRuleStorageManager.java:227) - Inserting 1 new Queue  Auto scaling  configuration: Logical Q rule id = 0 [ Producer limit per Exchange: 10000, Throughput limit per exchange:100000]
 INFO [Thread-2] (MngtServerStorage.java:302) - Inserting 1 new  autoscaling configuration
 INFO [Thread-2] (MngtServerStorage.java:303) - Inserting 3 3 3
ERROR [Thread-2] (MngtServerStorage.java:316) - Error while inserting new configuration
java.sql.SQLException: column Name is not unique
    at org.sqlite.DB.execute(DB.java:366)
    at org.sqlite.Stmt.exec(Stmt.java:56)
    at org.sqlite.Stmt.execute(Stmt.java:83)
    at org.roqmessaging.management.server.MngtServerStorage.addAutoScalingConfig(MngtServerStorage.java:308)
    at org.roqmessaging.management.server.MngtController.run(MngtController.java:440)
    at java.lang.Thread.run(Thread.java:679)
 INFO [Thread-2] (MngtServerStorage.java:276) - updateing 1 new logical Q configuration with auto scaling configuration name
 INFO [Thread-2] (MngtServerStorage.java:277) - Inserting in  test1 config:as1
DEBUG [Thread-2] (RoQBSONSerializer.java:228) - { "RESULT" : 0 , "COMMENT" : "The autoscaling as1 has been created for the Q test1"}
sskhiri commented 12 years ago

The excption is well caugth. This is an error log. I think the comment explains the error inthe command but it should return error i agree.

sskhiri commented 11 years ago

Here is what happens:

  1. The xchange and host scaling rules are created
  2. The rule configuration that points to those scaling rule cannot be created as the name already exist
  3. However, the rule configuration name (that already exists) is assigned to the queue, that is why the message says it is OK

Then the questions here are:

  1. Do we need to send a failure and not assigning the rule config name even if it exists?
  2. Do we need a rollback in this situation that will remove the xchange and host rule freshly created?

What is your view?

jbruggem commented 11 years ago

I'm not sure I grasp the exact consequences of what's happening internally, but it seems that any other solution than roll-back would lead to unpredictable behaviour for the client. If I understand correctly, in this situation we end up with:

So, if my assumptions are correct (they might not be), I see only two courses of actions:

Of course, offering both options to the client would also be a possibility ;-).

In any case from my point of view (as a client), when I send a single command I will tend to assume that an atomic operation is taking place behind the scenes, with a single result that I can predictably act upon.

sskhiri commented 11 years ago

I have implemented a security check that return an error in that case. In addition I have implemented rollback in case of problem during the transaction. Tested and validated.