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

[suggestion] Seperate Constants.java into some SubConstants Class #3137

Closed
cvictory opened this issue Jan 4, 2019 · 21 comments
Closed

[suggestion] Seperate Constants.java into some SubConstants Class #3137

cvictory opened this issue Jan 4, 2019 · 21 comments
Milestone

Comments

@cvictory
Copy link
Contributor

cvictory commented Jan 4, 2019

Currently we manage out constants values in org.apache.dubbo.common.Constants.java.

This class is very bloated and this structure is not well enough. Maybe we should categorize such constants into groups. In this way , we can add some XxxConstants, such as RegistryConstants,ConfigCenterConstants etc. It'd be better if it is split by module of dubbo-core.

@CrazyHZM
Copy link
Member

CrazyHZM commented Mar 7, 2019

@cvictory
Is there a plan for this suggestion? If there is no plan, I plan to start doing this in the near future.

@beiwei30
Copy link
Member

beiwei30 commented May 10, 2019

@CrazyHZM pls. take a look at #4017. I've finished step1.

  • step2, we need to create multiple Constants in dubbo-common module.
  • step3, start to reference new Constatns one by one.
  • step4, delete old Constants
  • step5, examine each new Constants and evaluate if they are more suitable to move out from dubbo-common into its delegated module, say: org.apache.dubbo.common.serialize.Constants to dubbo-serialization-api

Let me know if it's reasonable, and which part will you claim for step2. I will start from common:

    // BEGIN common
    public static final String DUBBO = "dubbo";
   ...
   public static final int MAX_PROXY_COUNT = 65535;
    // END common

@CrazyHZM
Copy link
Member

@beiwei30
I think this step is good, I am responsible for doing the remoting, rpc, config parts, I will start with remoting

@beiwei30
Copy link
Member

will you do it based on the current pull request, or wait until it's merged?

@CrazyHZM
Copy link
Member

Wait until after being merged

@beiwei30
Copy link
Member

@CrazyHZM #4017 is merged. pls. start to move constants into constants sub package first. In this step, let's don't touch org.apache.dubbo.common.Constants untouched.

@beiwei30
Copy link
Member

FYI, the first new constant class I created is: org.apache.dubbo.common.constants.CommonConstants

@CrazyHZM
Copy link
Member

I created
org.apache.dubbo.common.constants.RemotingConstants、
org.apache.dubbo.common.constants.RpcConstants and org.apache.dubbo.common.constants.ConfigConstants.

@beiwei30
Copy link
Member

beiwei30 commented May 10, 2019

@CrazyHZM see #4020

@CrazyHZM
Copy link
Member

@beiwei30
step2 #4021

@beiwei30
Copy link
Member

@beiwei30
step2 #4021

I left my comment. Would you mind to review mine too?

@CrazyHZM
Copy link
Member

@CrazyHZM see #4020

I will merge it when I pass ci

chickenlj pushed a commit that referenced this issue May 10, 2019
@beiwei30
Copy link
Member

I think we can now move to step3.

@CrazyHZM
Copy link
Member

I will start modifying the remoting later.

@beiwei30
Copy link
Member

beiwei30 commented May 14, 2019

so, now we should move to step4, start to review any constants suitable to move back to its own module.

for example, in RPCConstants:

    // BEGIN dubbo-rpc-rest
    String KEEP_ALIVE_KEY = "keepalive";

    boolean DEFAULT_KEEP_ALIVE = true;

    String EXTENSION_KEY = "extension";
    // END dubbo-rpc-rest

@CrazyHZM
Copy link
Member

CrazyHZM commented May 14, 2019

Add comments first or move directly?

@beiwei30
Copy link
Member

if you are pretty sure it should be move into individual module, just do it, and let's review :)

@CrazyHZM
Copy link
Member

ok,I will start to review remoting part.

@beiwei30
Copy link
Member

@CrazyHZM FYI, I am on remoting.

@CrazyHZM
Copy link
Member

@beiwei30
I will start the fourth step of config.

@cvictory
Copy link
Contributor Author

You can continue to work at this in dubbo-2.7.3

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

No branches or pull requests

3 participants