Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Facts should implement Serializable #159

Closed
mahendrakapadne opened this issue May 11, 2018 · 11 comments
Closed

Facts should implement Serializable #159

mahendrakapadne opened this issue May 11, 2018 · 11 comments

Comments

@mahendrakapadne
Copy link

mahendrakapadne commented May 11, 2018

Can we add support for rule engine to implement serializable interface

Caused by: java.io.NotSerializableException: org.jeasy.rules.api.Facts
Serialization stack:
- object not serializable (class: org.jeasy.rules.api.Facts, value: [])

@fmbenhassine fmbenhassine changed the title Caused by: java.io.NotSerializableException: org.jeasy.rules.api.Facts Facts should implement Serializable May 13, 2018
@fmbenhassine
Copy link
Member

fmbenhassine commented May 13, 2018

Thank you for raising this. Currently, Easy Rules is not usable in a distributed system. I will make Facts implement Serializable. Have you encountered any other exception in regards to this but related to another API? I'm thinking of Rules for example.

@mahendrakapadne
Copy link
Author

@benas : Thank you for quick reply. I will try to implement Serializable interface for Facts and other classes wherever it’s required and will let you know about it. I am currently in process of integrating easy-rules with Spark driver. My major focus is on using MVELRule which supports dynamic configurable behaviour.

@fmbenhassine
Copy link
Member

Great to hear Easy Rules being used with Spark. Please do not hesitate to share your experience, I'm curious!

Let me know which APIs need to be serializable and I'll make them serializable in the next version.

@mahendrakapadne
Copy link
Author

mahendrakapadne commented May 14, 2018

@benas Thank You. I got not Serializable exception in following classes. So I had implemented Serializable inteface for all these.

  1. Facts
  2. Rule
  3. Rules
  4. RuleEngine
  5. RuleListner
  6. RuleEngineListner
  7. RuleEngineParameters
  8. RuleDefinationValidator
  9. RuleProxy
  10. Action
  11. Condition

@mahendrakapadne
Copy link
Author

For MvelRule I am getting following exception,
java.lang.VerifyError: (class: ASMAccessorImpl_....., method: getKnownEgressType signature: ()Ljava/lang/Class;) Illegal type in constant pool

mikebrock/mvel#27

class EnrichFunction<T> implements Function<T> {
    Rules rules;
    RuleEngine ruleEngine;
    Facts facts;
    public EnrichFunction(Rules rules, RuleEngine ruleEngine, Facts facts) {
        this.rules = rules;
        this.ruleEngine = ruleEngine;
        this.facts = facts;
    }
    public <T> apply(T t) {
        facts.put("person", t);
        ruleEngine.fire(rules, facts);
        return t;
    }
}

@fmbenhassine
Copy link
Member

Thank you for your feedback and for giving the link to the mvel issue (this is out of scope of easy rules).

So if I understand correctly, only Facts needs to be Serializable. Is this correct?

@mahendrakapadne
Copy link
Author

Nope. I have listed all the classes in my previous comment.
Btw I tried using Spring SpEL instead of MVEL and it’s performance is better.

@mahendrakapadne
Copy link
Author

I would like to contribute for Spring EL rules

@fmbenhassine
Copy link
Member

I would like to contribute for Spring EL rules

Sure! you are welcome!

@fmbenhassine
Copy link
Member

Btw I tried using Spring SpEL instead of MVEL and it’s performance is better.

Spel Support has been added in #204 and will be included in the upcoming v3.3 release. Can you please give it a try with version 3.3.0.SNAPSHOT and let me know what do you think?

I'm asking for your help since you already tried with SpEL and your feedback will certainly be invaluable! Please add a comment if any in #204, let's keep this issue for the Serializable story.

Looking forward to your feedback.

@fmbenhassine
Copy link
Member

So I had implemented Serializable inteface for all these.

While I can understand that some classes can be changed to implement Serializable (like Facts and Rules), I don't understand the need to do it for interfaces (either by adding static final long serialVersionUID = 1L; to the interface or by making the interface extend Serializable).

When I evaluate the cost of change, it is actually more than the 11 classes/interfaces you listed. It should also be done for DefaultRulesEngine, InferenceRulesEngine, DefaultRuleListener, etc to be done correctly. That is almost 90% of production code. This is not something I'm planning to do in the short term and maintain in the long term. So I suggest to do this in a fork, OSS FTW!

For this reason, I'm closing the issue for now. Thank you anyway for raising it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants