-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Add TripleConfig to ProtocolConfig as nest configuration #14077
Conversation
@AlbumenJ Could you take a look add TripleConfig into configManager is reasonable?, or is there a better option? |
Add a special protocol related config to |
How about putting it in ProtocolConfig as nest configuration? |
|
Do you have any ideas? I think the second point is acceptable. |
If we change the full key from |
TripleConfig is newly added in version 3.3. Users should just add new configurations. |
If so, we can change it |
1258227
to
0a863ca
Compare
@EarthChen @oxsean @icodening PTAL |
dubbo-common/src/main/java/org/apache/dubbo/config/nested/TripleConfig.java
Outdated
Show resolved
Hide resolved
...dubbo-config-api/src/main/java/org/apache/dubbo/config/bootstrap/builders/TripleBuilder.java
Outdated
Show resolved
Hide resolved
dubbo-common/src/main/java/org/apache/dubbo/config/nested/TripleConfig.java
Outdated
Show resolved
Hide resolved
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
03abedb
to
dca464c
Compare
dubbo-config/dubbo-config-spring/src/main/resources/META-INF/dubbo.xsd
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@finefuture xsd should remove too.
@AlbumenJ LGTM.
There is a problem here. If the server-side and client-side protocols are inconsistent, configClientPipeline obtains the protocol name of the server side through url.getProtocol. At this time, ProtocolConfig will not be obtained from ConfigManager. I'm fixing it. |
This problem can be solved by using the application's own ProtocolConfig. If the user wants to configure triple, he can set it directly under a single protocol. When using multiple protocols, he can set the protocol for which TripleConfig is configured as the default protocol. return url.getOrDefaultApplicationModel().getApplicationConfigManager().getDefaultProtocols().stream()
.findFirst()
.flatMap(protocolConfig -> Optional.ofNullable(protocolConfig.getTriple()))
.orElseGet(TripleConfig::new); |
@EarthChen @icodening PTAL |
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
Embedded configuration may be better because it can be unified under the dubbo.protocol prefix. You can refer to MetricsConfig, and Prometheus configuration is also used as embedded configuration. If users do not want to use Prometheus, they can choose not to configure it. |
dubbo-common/src/main/java/org/apache/dubbo/config/context/ConfigManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
private TripleConfig getTripleConfig(URL url) { | ||
return url.getOrDefaultApplicationModel() | ||
.getApplicationConfigManager() | ||
.getOrAddProtocol(url.getProtocol()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use getOrAddProtocol
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the consumer side may not be configured with tri's ProtocolConfig, it needs to set a default tri's ProtocolConfig for it.
For example: https://github.com/apache/dubbo/blob/3.2/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/protocol/tri/TripleProtocolTest.java
LGTM. |
# Conflicts: # dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
What is the purpose of the change
Brief changelog
Verifying this change
Checklist