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

Fixes order problem in client user code deployment #16610

Merged

Conversation

sancar
Copy link
Contributor

@sancar sancar commented Feb 5, 2020

The issue is related to inheritance. If a a ChildClass is configured
first, it was trying to be defined first. It was searching for its
parent, and failing.
The issue is fixed by loading the parent class definition when needed,
from bundledClassDefinitions on the server.
(these are the whole classes send together from the client)

fixes #16575

(cherry picked from commit c9b0bcc)

@galibey
Copy link
Contributor

galibey commented Feb 5, 2020

@sancar But definition size doesn't correlate with inheritance. It depends only on amount of bytecode in the class. In your unit-tests, if we change ParentClass to something like this:

public class ParentClass implements Serializable, Runnable {

    @Override
    public void run() {

    }
}

tests will fail.

@sancar
Copy link
Contributor Author

sancar commented Feb 6, 2020

Yes, you are right. I assumed that child will always have the bytecode of its parent, and some more on top of it. Let me think another way to solve this.

@puzpuzpuz
Copy link
Contributor

I think, the correct order of classes (hierarchy) can be determined via Reflection API.

The issue is related to inheritance. If a a ChildClass is configured
first, it was trying to be defined first. It was searching for its
parent, and failing.
The issue is fixed by sorting the class definitions by size
in ascending order. This way, we will make sure that a parent class
will be defined first.

fixes hazelcast#16575

(cherry picked from commit c9b0bcc)
@sancar sancar force-pushed the fix/clientUserCodeDeploymentOrder/3.12.z branch from 8716a86 to cb371fc Compare February 7, 2020 09:58
@sancar
Copy link
Contributor Author

sancar commented Feb 7, 2020

@ihsandemir @galibey @galibey I have updated the pr for the inheritance order problem.

for (String className : classNames) {
classes.add(configClassLoader.loadClass(className));
}
List<Class> sortedClasses = sortByInheritance(classes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this also do the same?

        Collections.sort(classes, (c1, c2) -> c1.equals(c2) ? 0 : c1.isAssignableFrom(c2) ? -1 : 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a correct comparator implementation, because it does not give a total order. See this question and answer
https://stackoverflow.com/questions/50921813/how-to-sort-classes-according-to-their-inheritance-level#comment88888102_50922334

@galibey
Copy link
Contributor

galibey commented Feb 7, 2020

The fix doesn't cover classes loaded from jars. The issue is relevant for them as well.

@sancar sancar changed the title Fixes order problem in client user code deployment [WIP] Fixes order problem in client user code deployment Feb 10, 2020
The issue is related to inheritance. If a a ChildClass is configured
first, it was trying to be defined first. It was searching for its
parent, and failing.
The issue is fixed by loading the parent class definition when needed,
from bundledClassDefinitions on the server.
(these are the whole classes send together from the client)

fixes hazelcast#16575

(cherry picked from commit 15fcc57)
@sancar sancar changed the title [WIP] Fixes order problem in client user code deployment Fixes order problem in client user code deployment Feb 10, 2020
@sancar
Copy link
Contributor Author

sancar commented Feb 10, 2020

@galibey I have checked the jars. Indeed, they are also causing the same problem. I changed my solution after realizing that. Can you review my solution in the last commit ?

@galibey
Copy link
Contributor

galibey commented Feb 10, 2020

@sancar Good solution 👍

@galibey galibey self-requested a review February 10, 2020 17:43
@sancar sancar merged commit b35d581 into hazelcast:3.12.z Feb 11, 2020
@sancar sancar deleted the fix/clientUserCodeDeploymentOrder/3.12.z branch February 11, 2020 10:39
@mmedenjak mmedenjak added the Source: Internal PR or issue was opened by an employee label Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java.lang.NoClassDefFoundError is thrown when the user uploaded classes come in a wrong order
5 participants