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

Multiple environmental isolation #1075

Merged
merged 35 commits into from
May 28, 2019
Merged

Multiple environmental isolation #1075

merged 35 commits into from
May 28, 2019

Conversation

XCXCXCXCX
Copy link
Contributor

Switch environments by specifying env environment variables, let seata use different environment configurations.

example:
past
+file.conf
+registry.conf

now
+file-dev.conf
+registry-dev.conf
+file.conf
+registry.conf

Use -Denv=dev to recognize the suffix, and choose registry-dev.conf configuration.
when -Denv=default or null, use registry.conf.
when -Denv=unknown, use NULL configuration, at this point some objects will report an error due to missing configuration.

@codecov-io
Copy link

codecov-io commented May 20, 2019

Codecov Report

Merging #1075 into develop will decrease coverage by 0.01%.
The diff coverage is 75%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1075      +/-   ##
=============================================
- Coverage      41.87%   41.85%   -0.02%     
  Complexity      1361     1361              
=============================================
  Files            243      243              
  Lines          10118    10118              
  Branches        1325     1325              
=============================================
- Hits            4237     4235       -2     
  Misses          5333     5333              
- Partials         548      550       +2
Impacted Files Coverage Δ Complexity Δ
...a/io/seata/discovery/registry/RegistryFactory.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ery/registry/consul/ConsulRegistryServiceImpl.java 4.76% <100%> (ø) 1 <1> (ø) ⬇️
...very/registry/zk/ZookeeperRegisterServiceImpl.java 63.77% <100%> (ø) 24 <0> (ø) ⬇️
...covery/registery/etcd/EtcdRegistryServiceImpl.java 14.4% <100%> (ø) 3 <0> (ø) ⬇️
...server/store/file/FileTransactionStoreManager.java 45.64% <0%> (-0.7%) 19% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71216f6...c93d1cf. Read the comment docs.

@xingfudeshi
Copy link
Member

Good!

/**
* The constant FILE_INSTANCE.
*/
public static final Configuration FILE_INSTANCE = new FileConfiguration(REGISTRY_CONF);
private static final String ENV_VALUE = System.getProperty("env");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can provide a util class to set or resolve the env value
default it can fetch from system properties
when users use spring ,maybe they want to config from the application.properties ,so they can invoke your util method instead of use System.setProperty or pass by -D

@xingfudeshi
Copy link
Member

@leizhiyuan To read configs from application,it's better to create a module separately to do that.like seata-spring-boot-starter.

Copy link
Contributor

@leizhiyuan leizhiyuan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

LGTM

@xingfudeshi xingfudeshi self-requested a review May 23, 2019 01:22
Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

There are conflict files.

@xingfudeshi
Copy link
Member

This kind of configuration isolation is acceptable on the server side, but may not be so good on the client side, such as spring-boot.

@xingfudeshi
Copy link
Member

I think an environment variable can be added. The name is SEATA_CONFIG_ENV. Before loading the configuration file, both the server and the client should read the environment variable of the current System (using system.getenv).If it's not blank then load the corresponding configuration files, such as registry-dev.conf and file-dev.conf, where dev is the value read from the environment variable.

@XCXCXCXCX
Copy link
Contributor Author

I think an environment variable can be added. The name is SEATA_CONFIG_ENV. Before loading the configuration file, both the server and the client should read the environment variable of the current System (using system.getenv).If it's not blank then load the corresponding configuration files, such as registry-dev.conf and file-dev.conf, where dev is the value read from the environment variable.

Good idea.

}

private static final Configuration DEFAULT_FILE_INSTANCE = new FileConfiguration(REGISTRY_CONF_PREFIX + REGISTRY_CONF_SUFFIX);
public static final Configuration CURRENT_FILE_INSTANCE = (ENV_VALUE == null || "default".equals(ENV_VALUE)) ? DEFAULT_FILE_INSTANCE : new FileConfiguration(REGISTRY_CONF_PREFIX + "-" + ENV_VALUE + REGISTRY_CONF_SUFFIX);
Copy link
Member

Choose a reason for hiding this comment

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

it's better to define a constant for "default".

Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

Thanks.LGTM.

@xingfudeshi xingfudeshi added this to the 0.7.0 milestone May 27, 2019
@xingfudeshi xingfudeshi requested a review from slievrly May 27, 2019 04:24
@xingfudeshi xingfudeshi added the type: feature Category issues or prs related to feature request. label May 27, 2019
@funky-eyes
Copy link
Contributor

由于前期Seata社区治理规范问题部分代码作者未签署CLA,可能引发社区知识产权风险问题。请所有在Seata社区贡献过代码(包含:主项目、官网、samples和多语言项目等)的 contributor 帮忙在这个链接登录github账号签署相应的开发者CLA,https://cla-assistant.io/seata/seata 。2023.1.31 未签署CLA的代码将会被rewrite,拜托大家帮忙签一下。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Category issues or prs related to feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants