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

Conflict with some mechanism in Dubbo 2.6.2 #1769

Closed
3 of 4 tasks
3zamn opened this issue Oct 16, 2018 · 34 comments
Closed
3 of 4 tasks

Conflict with some mechanism in Dubbo 2.6.2 #1769

3zamn opened this issue Oct 16, 2018 · 34 comments
Labels
agent Language agent related. bug Something isn't working and you are sure it's a bug! plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Milestone

Comments

@3zamn
Copy link

3zamn commented Oct 16, 2018

Please answer these questions before submitting your issue.

  • Why do you submit this issue?
  • Question or discussion
  • Bug
  • Requirement
  • Feature or performance improvement

Question

  • What do you want to know?
    extension.SpringExtensionFactory - [DUBBO] No spring extension(bean) named:skyWalkingDynamicField, try to find an extension(bean) of type java.lang.Object, dubbo version: oss, current host: 192.168.0.3
    ERROR extension.ExtensionLoader - [DUBBO] fail to inject via method setSkyWalkingDynamicField of interface com.alibaba.dubbo.rpc.Filter:

Bug

  • Which version of SkyWalking, OS and JRE?
    SkyWalking5.6.0、dubbo2.6.2、jre1.8

  • Which company or project?

  • What happen?
    If possible, provide a way for reproducing the error. e.g. demo application, component version.
    dubbo2.6.0- is ok、but dubbo2.6.2+ didn't support


Requirement or improvement

Please support dubbo2.6.2+

@wu-sheng wu-sheng added bug Something isn't working and you are sure it's a bug! agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Oct 16, 2018
@wu-sheng
Copy link
Member

@ascrutae please take a look at this.

@3zamn 3zamn changed the title It does not dubbo v2.6.2+? It does not support dubbo v2.6.2+? Oct 17, 2018
@IcebergXTY
Copy link

IcebergXTY commented Oct 23, 2018

@wu-sheng Hey, I also met this problem and I found the reason. Because Skywalking will add setSkyWalkingDynamicField(Object) to class MonitorFilter and this conflicts with dubbo's spi extention mechanism.

/*
 * com.alibaba.dubbo.common.extension.ExtensionLoader
 * Here dubbo's spi extention mechanism will traverse all the public set method
 */
private T injectExtension(T instance) {
    try {
        if (objectFactory != null) {
            for (Method method : instance.getClass().getMethods()) {
                if (method.getName().startsWith("set")
                        && method.getParameterTypes().length == 1
                        && Modifier.isPublic(method.getModifiers())) {
                    Class<?> pt = method.getParameterTypes()[0];
                    try {
                        String property = method.getName().length() > 3 ? method.getName().substring(3, 4).toLowerCase() + method.getName().substring(4) : "";
                        Object object = objectFactory.getExtension(pt, property);
                        if (object != null) {
                            method.invoke(instance, object);
                        }
                    } catch (Exception e) {
                        logger.error("fail to inject via method " + method.getName()
                                + " of interface " + type.getName() + ": " + e.getMessage(), e);
                    }
                }
            }
        }
    } catch (Exception e) {
        logger.error(e.getMessage(), e);
    }
    return instance;
}

Before dubbo 2.6.3, the extentionFactory just get bean by name, so there is no errro.

/*
 * com.alibaba.dubbo.config.spring.extension.SpringExtensionFactory
 */
@Override
@SuppressWarnings("unchecked")
public <T> T getExtension(Class<T> type, String name) {
    for (ApplicationContext context : contexts) {
        if (context.containsBean(name)) {
            Object bean = context.getBean(name);
            if (type.isInstance(bean)) {
                return (T) bean;
            }
        }
    }
    return null;
}

After, dubbo monify the code, it also get bean by type and this throw the exception.

@Override
@SuppressWarnings("unchecked")
public <T> T getExtension(Class<T> type, String name) {
    for (ApplicationContext context : contexts) {
        if (context.containsBean(name)) {
            Object bean = context.getBean(name);
            if (type.isInstance(bean)) {
                return (T) bean;
            }
        }
    }

    logger.warn("No spring extension(bean) named:" + name + ", try to find an extension(bean) of type " + type.getName());

    for (ApplicationContext context : contexts) {
        try {
            return context.getBean(type);
        } catch (NoUniqueBeanDefinitionException multiBeanExe) {
            //here will throw exception because setSkyWalkingDynamicField() type is Object and spring will find many beans
            throw multiBeanExe;
        } catch (NoSuchBeanDefinitionException noBeanExe) {
            if (logger.isDebugEnabled()) {
                logger.debug("Error when get spring extension(bean) for type:" + type.getName(), noBeanExe);
            }
        }
    }

    logger.warn("No spring extension(bean) named:" + name + ", type:" + type.getName() + " found, stop get bean.");

    return null;
}

Emmm....Maybe you can communicate with dubbo....

@wu-sheng
Copy link
Member

@IcebergXTY Thanks for the feedback. I already dropped an request in Dubbo mail list, https://lists.apache.org/thread.html/ed79a0284fddfedc53c96e5666a059617693cd4d7628c32de2935ea4@%3Cdev.dubbo.apache.org%3E . Let's wait and see what are they going to response, and we could work from here.

@diecui1202
Copy link

@chickenlj I've checked the git commit log, this change is from you. Could u pls. take some time to look at it?

@chickenlj
Copy link

Sure, I am here on it.

I guess the newly added method setSkyWalkingDynamicField(Object) from SkyWalking is not expected to be automatically injected by Dubbo framework. To make Dubbo more adaptable, I think we should do the following:

  1. A method level annotation to enable auto-injection or not.
  2. Special handling for generic Object parameter type.

@chickenlj
Copy link

@IcebergXTY @wu-sheng Do you think this will solve your problem?

@wu-sheng
Copy link
Member

A method level annotation to enable auto-injection or not.

@chickenlj I hope the default is not :) SkyWalking will add this method to all enhanced classes, we can't add an annotation belongs to Dubbo only, may trigger ClassNotFound in other scenarios.

Or, when could not find the inject way, just ignore. The auto set should not be active always from my perspective.

@chickenlj
Copy link

A method level annotation to enable auto-injection or not.

@chickenlj I hope the default is not :) SkyWalking will add this method to all enhanced classes, we can't add an annotation belongs to Dubbo only, may trigger ClassNotFound in other scenarios.

Set the default value to not would bring lots of changes to Dubbo, many methods would need to add an annotation and enable injection. Can't SkyWalking just add an annotation when instrumenting Dubbo?
Anyway, I think Dubbo need an annotation to flexibly control the injection action.

Or, when could not find the inject way, just ignore. The auto set should not be active always from my perspective.

Sounds reasonable, I will consider the possibility of ignoring it when fails.

@ascrutae
Copy link
Member

@chickenlj What is The parameter instance about injectExtension method, an MonitorFilter object?

@wu-sheng
Copy link
Member

Set the default value to not would bring lots of changes to Dubbo, many methods would need to add an annotation and enable injection. Can't SkyWalking just add an annotation when instrumenting Dubbo?
Anyway, I think Dubbo need an annotation to flexibly control the injection action.

In theory, we can. But we haven't that kind of core API to do so. So, I hope Dubbo can ignore that by the default mechanism. @chickenlj @diecui1202 Hope this could be fixed soon, and I will keep this open to track the status. If you have any solution, please leave the message here.

@chickenlj
Copy link

I have created a patch to fix this problem, briefly changes:

  • Add optional Inject annotation for developers to decide open auto-injection or not.
  • Printing log instead of throwing Exception in SpringExtensionFactory when multi-extension or no extension found.

@wu-sheng
Copy link
Member

Both of these are great. Especially just log rather than throw exception. :) Thank so much for helping. @chickenlj .

FYI @3zamn @IcebergXTY @ascrutae I think we just need to wait for new version of Dubbo. @chickenlj please info us after release done, then we can close this issue officially.

@wu-sheng wu-sheng changed the title It does not support dubbo v2.6.2+? Conflict with some mechanism in Dubbo 2.6.2 Oct 24, 2018
@IcebergXTY
Copy link

@wu-sheng That's great!Thank you for your work! 😊

@wu-sheng
Copy link
Member

@IcebergXTY The credits belong to Dubbo team. @chickenlj

@chickenlj
Copy link

I think it will be released along with the upcoming Dubbo v2.6.5.

@wu-sheng
Copy link
Member

I think it will be released along with the upcoming Dubbo v2.6.5.

@chickenlj Great to know. @IcebergXTY When you have time, please help to test SkyWalking on their incoming 2.6.5, then provide the feedback here.

@IcebergXTY
Copy link

@wu-sheng @chickenlj I have updated to dubbo-2.6.5. It works well now. Excellent!

@wu-sheng
Copy link
Member

@IcebergXTY @chickenlj 2.6.5 is already released or not?

@wu-sheng
Copy link
Member

@ascrutae we need to add new dubbo version to our test cases. And we could remove some older ones.

@IcebergXTY
Copy link

@wu-sheng I‘m not sure whether dubbo has released officially but the maven repository has already contained the dubbo-2.6.5

@chickenlj
Copy link

The new Dubbo version 2.6.5 with this issue solution included has just been released.
Please help to check.
https://github.com/apache/incubator-dubbo/releases/tag/dubbo-2.6.5

@wu-sheng wu-sheng added this to the 6.0.0-beta milestone Nov 23, 2018
@wu-sheng
Copy link
Member

Great. Closing this. Supported is back in 2.6.5 release.

@qixiaobo
Copy link

@chickenlj @wu-sheng Sad……Because dubbo 2.6.6 has bugs so I degrade to dubbo 2.6.4……But I meet this problem. I meet the problem as same as 2009 which QQ or 360 I must choose at most one……

apache/dubbo#4612

@wu-sheng
Copy link
Member

They are talking about 2.6.7 is available, is that a better choice? This is also a Dubbo bug... sadly.

@qixiaobo
Copy link

They are talking about 2.6.7 is available, is that a better choice? This is also a Dubbo bug... sadly.

Sad……
2.6.X merges my pr…… But 2.6.7 never ……
image
image
Maybe 2.6.8…… I cannot wait for it

@wu-sheng
Copy link
Member

Apache can't randomly release, and each release is unchangeable. So... I am afraid Dubbo community did the right thing. Only the next release is an option. We could ask a Dubbo committer do patch release, such as 2.6.7-patch1, if the community accepts that.

@qixiaobo
Copy link

You are right, so spring-cloud-alibaba comes back to alibaba repo. Thanks for replying

@qixiaobo
Copy link

How about I add a spring bean named skyWalkingDynamicField to avoid this problem ? @wu-sheng In this mechanism maybe skywalking never need this field?

@wu-sheng
Copy link
Member

SkyWalking plugin requires this field. I doubt this w/ highly possible risks.

@qixiaobo
Copy link

SkyWalking plugin requires this field. I doubt this w/ highly possible risks.

I have no other choices since dubbo 2.6.5+ has a bug cannot serialize and 2.6.8 is not release yet…… Maybe I can try?

@wu-sheng
Copy link
Member

You could try :) I am just concerned that, it will break the SpringMVC plugin.

@qixiaobo
Copy link

You could try :) I am just concerned that, it will break the SpringMVC plugin.

Or maybe I can delete the dubbo plugin?[It may works]
Also I consider about delete the dubbo Monitorfilter [Skywalking just enchace this class]--->The same result with delete dubbo plugin

@qixiaobo
Copy link

/**
 * https://github.com/apache/skywalking/issues/1769
 */
@Bean
public String skyWalkingDynamicField() {
    return "avoid skywalking conflicts with dubbo 2.6.5-";
}

I add this code to my spring config , and it works~

@wu-sheng
Copy link
Member

That is luck :) Hope it would break other things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. bug Something isn't working and you are sure it's a bug! plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Development

No branches or pull requests

7 participants