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

SOFABoot quick start documentation issue #1031

Open
nobodyiam opened this issue Apr 19, 2021 · 17 comments
Open

SOFABoot quick start documentation issue #1031

nobodyiam opened this issue Apr 19, 2021 · 17 comments
Labels
good first issue Good for newcomers remind To be further discussed

Comments

@nobodyiam
Copy link
Member

nobodyiam commented Apr 19, 2021

I walked through the SOFABoot quick start guides and have some suggestions.

1. It's not always easy to change the parent pom, so it's better to document an alternative way, e.g. import the sofaboot-dependencies in dependencyManagement

  <dependencyManagement>
    <dependencies>
      <dependency>
        <groupId>com.alipay.sofa</groupId>
        <artifactId>sofaboot-dependencies</artifactId>
        <version>${sofa.boot.version}</version>
        <type>pom</type>
        <scope>import</scope>
      </dependency>
    </dependencies>
  </dependencyManagement>

2. XML configuration is not widely used now, we'd better update the quick start documents using annotation examples

3. The annotation should be more user friendly

Take the example below:

  1. after the @SofaService is annotated, the @Component should not be necessary
  2. The interfaceType is not mandatory, our sample could remove it
  3. It's not good to let users specify binding type as string, it's better to provide some enums for bindings.
@SofaService(interfaceType = HelloSyncService.class, bindings = {@SofaServiceBinding(bindingType = "bolt")})
@Component
public class HelloSyncServiceImpl implements HelloSyncService {
  ...
}

4. Registry instructions should be added in the quick start doc

Currently there is no such instruction, so the user could not run the demo following the quick start doc from scratch.

@EvenLjj EvenLjj added the good first issue Good for newcomers label Jul 22, 2021
@seeflood
Copy link
Member

seeflood commented Nov 11, 2021

  1. It's not good to let users specify binding type as string, it's better to provide some enums for bindings.
@SofaService(interfaceType = HelloSyncService.class, bindings = {@SofaServiceBinding(bindingType = "bolt")})
@Component
public class HelloSyncServiceImpl implements HelloSyncService {
  ...
}

We can make it a community task.

Update 11.19: This task will be assigned to @IrvingOS

@Kunple-w
Copy link
Contributor

如果类上@SofaService标记后@Component不是必须的, 那么方法上@SofaService标记后@Bean应该也不是必须的吧?

@seeflood
Copy link
Member

seeflood commented Dec 19, 2021

@Kunple-w 恩恩 我理解是想改成这样的。用户只需要写 @SofaService 这一个注解,省的写两个

@Kunple-w
Copy link
Contributor

@seeflood 我的想法是类似于Spring的@Bean@Component/@Service的定位,应该有2个注解; 比如@SofaBean@SofaService,而SofaService取消掉ElementType.METHOD的支持,这样的话SofaService有兼容性问题。
这样合适吗?

@renliangyu857
Copy link

after the @SofaService is annotated, the @component should not be necessary
please assign to me, thanks

@seeflood
Copy link
Member

seeflood commented Dec 21, 2021

@Kunple-w 不能向下兼容的话,就不合适了~因为有很多生产用户在用,不兼容的话用户体验比较差。
我没细看过相关代码,不确定该咋搞,但听起来这个需求可以参考spring的组合注解来实现?我记得spring有那种一个注解A能拿来当注解B/C/D用的机制

@seeflood
Copy link
Member

seeflood commented Dec 21, 2021

after the @SofaService is annotated, the @component should not be necessary please assign to me, thanks

Cool ! Could u please describe your design on how to implement it?
It's better we have a design discussion before coding, which avoids rework.
(You can write in either english or chinese,we don't mind

@Kunple-w
Copy link
Contributor

@seeflood 实现方式有多种,

  1. 组合注解,正如你说的,如@SofaService上添加@Component注解,改动量小,缺点就是@SofaService就和spring绑定到一起了,如果sofa的方向就是这个方向,这个倒也合适;
  2. 自定义后置处理器解析@SofaService,类似于现在的方式,但是额外需要处理被注解bean放到cxt的逻辑,比组合注解改动量大一些,特点是该注解就和spring关系不大,只是sofa中定义的注解,可以独立处理;
    我理解1和2是对注解的定位略有差异,方向是需要确定的。

@EvenLjj
Copy link
Collaborator

EvenLjj commented Dec 21, 2021

@Kunple-w 考虑的还是很全面的,这个目前非开源版也做了些尝试,是通过组合注解的方式来实现的。因为SOFABoot中主要也是会强依赖spring相关的组件,所以推荐方式一,简单可行。

@nogeek-cn
Copy link

nogeek-cn commented Dec 21, 2021

一开始我想认领这个 @SofaService 的任务。

我的想法是,

  • 可以参考 Dubbo 的 ServiceAnnotationPostProcessor 如何做的

    1. 实现:BeanDefinitionRegistryPostProcessor 接口,实现 postProcessBeanDefinitionRegistry方法 在这个方法内部来做扫描,注册到 Spring 容器中 的 Bean
    2. 通过,实现 ClassPathBeanDefinitionScanner,ClassPathBeanDefinitionScanner#scan(String...) 方法可以扫描出来 Bean, 然后注册到容器中
    3. 后边才走到 构造动态 SDK 等。

扫描要实现,对对应的包进行扫描。有两种实现方式。

  1. 如何拿到 扫描 @SofaService 的 scanerPackageList 可以参考 https://github.com/Nepxion/Discovery/blob/6.x.x/discovery-plugin-strategy/discovery-plugin-strategy-starter/src/main/java/com/nepxion/discovery/plugin/strategy/extractor/StrategyPackagesExtractor.java 内部使用 AutoConfigurationPackages#get(BeanFactory) 拿到了 scan 的包的列表

    • 但是很遗憾。。 org.springframework.boot.autoconfigure.AutoConfigurationPackages 类在, Maven: org.springframework.boot:spring-boot-autoconfigure:2.3.9.RELEASE , 对应模块 并没有依赖,导致没有办法用
    • 所以这点难点,让我不知道该怎么办了。
  2. dubbo 的 xml 配置里边有 dubbo:basePackages 扫描包,annotation 有 @EnableDubbo#scanBasePackages ,所以如果要这个东西如何配置进去,还是一个问题 ...

其实我比较倾向于 用 AutoConfigurationPackages#get(BeanFactory) 返回 List<String> 来做统一的抽象,这样子,更加统一一点,毕竟要做开源统一

可以讨论一下是否可以添加对应的依赖,然后,这个方案是否可以尝试着做一下的必要。:smile: @EvenLjj E @seeflood

至于说用组合注解的方式的话,和 Spring 绑定的太深了。

@EvenLjj
Copy link
Collaborator

EvenLjj commented Dec 24, 2021

@Darian1996 你的方案应该就是倾向于 @Kunple-w 上面所讲到的方案二,扫描注解已经在com.alipay.sofa.runtime.spring.ServiceBeanFactoryPostProcessor 中做了,只需要将其注册到 Spring 容器中应该就是可以了。这个需求可能还要关注的一点,不影响原来的@SofaService的使用,保持兼容性可能会成为影响这个方案设计的一个重要因素。

@nogeek-cn
Copy link

nogeek-cn commented Dec 24, 2021

@Darian1996 你的方案应该就是倾向于 @Kunple-w 上面所讲到的方案二,扫描注解已经在com.alipay.sofa.runtime.spring.ServiceBeanFactoryPostProcessor 中做了,只需要将其注册到 Spring 容器中应该就是可以了。这个需求可能还要关注的一点,不影响原来的@SofaService的使用,保持兼容性可能会成为影响这个方案设计的一个重要因素。

@EvenLjj

扫描注解没有是前提

@Override
    public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException {
        Arrays.stream(beanFactory.getBeanDefinitionNames())
            .collect(Collectors.toMap(Function.identity(), beanFactory::getBeanDefinition))
            .forEach((key, value) -> transformSofaBeanDefinition(key, value, beanFactory));
    }

这里是通过 @Service 注册到 Spring 容器中,然后 每一个 Bean 都去进行看一下是否发布 Sofa服务

 private void transformSofaBeanDefinition(String beanId, BeanDefinition beanDefinition,
                                             ConfigurableListableBeanFactory beanFactory) {
        if (BeanDefinitionUtil.isFromConfigurationSource(beanDefinition)) {
            generateSofaServiceDefinitionOnMethod(beanId, (AnnotatedBeanDefinition) beanDefinition,
                beanFactory);
        } else {
            Class<?> beanClassType = BeanDefinitionUtil.resolveBeanClassType(beanDefinition);
            if (beanClassType == null) {
                SofaLogger.warn("Bean class type cant be resolved from bean of {}", beanId);
                return;
            }
            generateSofaServiceDefinitionOnClass(beanId, beanClassType, beanDefinition, beanFactory);
        }
    }

所以,是没有缺失了先扫描 @SofaService 的过程 ~~~

BeanFactoryPostProcessor#postProcessBeanFactory 的这个,是注册到容器才会触发的。所以,扫描注解没有 ... ...

旧方法

以前的方法 @Service 注册到 容器 , 从容器中拿出来,遍历,发布 SOFA 服务。

新方式

扫描 @SofaService 注册容器, 这一步才是难点。

直到这个,才能到我的提出的需要扫描 包的方案,继续往下讨论。

所以,前提是去掉 @Service 的自动注册到 Spring 容器,增加 @SofaService 的自动注册到 Spring 容器

@ridKnight
Copy link

ridKnight commented Dec 28, 2021

直接给SofaService类加上@Component@Indexed注解就行了啊,这两个是meta annotation,被注解的其他注解会自动携带这两个注解

@renliangyu857
Copy link

直接给SofaService类加上@Component@Indexed注解就行了啊,这两个是meta annotation,被注解的其他注解会自动携带这两个注解

学习了,没必要搞扫描那些,太复杂了

@nogeek-cn
Copy link

直接给SofaService类加上@Component@Indexed注解就行了啊,这两个是meta annotation,被注解的其他注解会自动携带这两个注解

这只是实现的方式,前提是,想不想把 Sofa 和 Spring 绑定在一起

@stale
Copy link

stale bot commented Feb 27, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Feb 27, 2022
@nobodyiam
Copy link
Member Author

Is there any update to this issue?

@stale stale bot removed the wontfix This will not be worked on label Feb 28, 2022
@EvenLjj EvenLjj added the remind To be further discussed label Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers remind To be further discussed
Projects
None yet
Development

No branches or pull requests

7 participants