[ROCKETMQ-135] Broker cannot be properly finalized on failure to load…

[shroman]

… a storage plugin.

JIRA issue: https://issues.apache.org/jira/browse/ROCKETMQ-135

[shroman]

Folks, MessageStoreFactory#build returns only one store instance. Does it make any sense to iterate over the list of plugins in the configuration?
How about removing that for loop and just try to instantiate from the trimmed plugin string? (See my next commit below)

[coveralls]

Coverage increased (+0.04%) to 31.027% when pulling dde713c on shroman:ROCKETMQ-135 into e3f4251 on apache:develop.

[coveralls]

Coverage increased (+0.04%) to 31.027% when pulling dde713c on shroman:ROCKETMQ-135 into e3f4251 on apache:develop.

[coveralls]

Coverage increased (+0.04%) to 31.027% when pulling dde713c on shroman:ROCKETMQ-135 into e3f4251 on apache:develop.

[coveralls]

Coverage increased (+0.1%) to 31.087% when pulling dde713c on shroman:ROCKETMQ-135 into e3f4251 on apache:develop.

[Jaskey]

@shroman user may have several plugins as Decorator Patterns, and build method returns the outtest store decorator, which in my opinion is right.

[shroman]

@Jaskey For a decorated class, you don't write "DecoratedClassA, DecoratedClassB, DecoratedClassC" in your properties, do you? :) You just specify your outer class, say "DecoratedClassC", and initialize. Inheritance and composition are done for you.

The current code explicitly initializes all classes, and use only the latest one.

[Jaskey]

@shroman
Sorry, I still cannot see the problem. No initialization are called inside the build method and only the outer class are returned.

Do you concern about the properties ways or the code implementations or the functionality?

If you are concerning about the property way, may be we can discuss more about it.

But supportive to list of plugins instead of only one will help user easily to pile up their plugin components.

As to the decorator pattern way, for example , user has three decorator plugins say DecoratedClassA, DecoratedClassB, DecoratedClassC,

when we write code we must write something like:

MessageStore c = //something here;
DecoratedClassA c1 = new DecoratedClassA (c );
DecoratedClassB c2 = new DecoratedClassB (c1 );
DecoratedClassC c3 = new DecoratedClassB (c2 );

and we just use c3 to do our jobs, which is exactly the same way as the current implementations except that rocketmq plugins have to use reflection to load class.

And if only one plugin can be supported, the decorator has to be done by the developers, which is not friendly enough in my opinion

[shroman]

@Jaskey For this, write your decorated code (what you write above) in, say, MyDecoratedClass and specify "MyDecoratedClass" as your storage to initialize. That will suffice.
It's up to the user how he/she implements the store plugin class (through decorators or whatever), what we do is just instantiate it from the class name.

[Jaskey]

@shroman , indeed it works.But this can be.done by rocketmq actually to make it easier to make up.everything.

Say user may have to plugin two store pligins , one for mysql, one for kafka . And after days, the kafka plugin is no longer needed for a period of time, they just modify the property file to remove that plugin and the jobs will be done by rocketmq. If only one plugin is supported the need to modify the source code to provide a.new plugin don't they？

[shroman]

I would rather get suggestions about this modification, if any, instead of hypothetical considerations. What is described here cannot be done with the current RocketMQ code base -- it's not just config changes, you will need to restart the broker cluster. In addition, from the code I cannot see that more than one store at a time can be supported. Can anyone confirm?

[Jaskey]

@shroman

From the existing implementations, the author are apparently try to support more than one plugin easily.

Maybe we can discuss about this with a separate issue in the mail list, for now we just focus on the bug here.

[shroman]

Initializing a bunch of store implementations and returning only one store makes no sense too.

@vongosling @zhouxinyu @lizhanhui

[vongosling]

@shroman I will close the pr, if you happened to the same question, please let me know.