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

Mostly modification to satisfy more complexing support scenrio such as service Level model #3264

Merged
merged 15 commits into from Feb 19, 2019

Conversation

Jeff-Lv
Copy link
Contributor

@Jeff-Lv Jeff-Lv commented Jan 17, 2019

  • add a attributeMap to Invocation
  • rename the varibale name NAME_ID_MAP
  • add a method for class CodecSupport
  • import servicemetada and associated it with models class

@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #3264 into 3.x-dev will decrease coverage by 0.05%.
The diff coverage is 58.44%.

Impacted file tree graph

@@              Coverage Diff              @@
##             3.x-dev    #3264      +/-   ##
=============================================
- Coverage      63.81%   63.75%   -0.06%     
  Complexity        75       75              
=============================================
  Files            654      655       +1     
  Lines          28377    28416      +39     
  Branches        4806     4808       +2     
=============================================
+ Hits           18108    18116       +8     
- Misses          7993     8022      +29     
- Partials        2276     2278       +2
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/alibaba/dubbo/rpc/Invocation.java 40% <0%> (-10%) 0 <0> (ø)
...in/java/org/apache/dubbo/config/ServiceConfig.java 54.75% <100%> (ø) 0 <0> (ø) ⬇️
.../java/org/apache/dubbo/config/ReferenceConfig.java 58.82% <100%> (-0.37%) 0 <0> (ø)
...a/org/apache/dubbo/rpc/model/ApplicationModel.java 71.42% <12.5%> (-23.58%) 0 <0> (ø)
.../main/java/org/apache/dubbo/rpc/RpcInvocation.java 61.11% <25%> (-1.68%) 0 <0> (ø)
.../dubbo/registry/integration/RegistryDirectory.java 79.35% <40%> (-0.65%) 0 <0> (ø)
...java/org/apache/dubbo/rpc/model/ConsumerModel.java 61.53% <40%> (-12.15%) 0 <0> (ø)
...rg/apache/dubbo/rpc/model/ConsumerMethodModel.java 80% <50%> (-1.4%) 0 <0> (ø)
...va/org/apache/dubbo/rpc/model/ServiceMetadata.java 55% <55%> (ø) 0 <0> (?)
...rg/apache/dubbo/rpc/model/ProviderMethodModel.java 83.33% <66.66%> (ø) 0 <0> (ø) ⬇️
... and 15 more

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 a27c5a7...72e2c7c. Read the comment docs.

@Jeff-Lv Jeff-Lv changed the title Mostly modification to satisfy the Alibaba's HSF support Mostly modification to satisfy more complexing support scenrio such as service Level model Jan 17, 2019
@@ -83,4 +83,9 @@
*/
Invoker<?> getInvoker();

Object put(Object key, Object value);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we these interface level method documentation? what do you say?

@@ -113,6 +115,14 @@ public void setInvoker(Invoker<?> invoker) {
this.invoker = invoker;
}

public Object put(Object key, Object value) {
return attributes.put(key, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to consider any thread safety here?
should we not prevent addition of adding NULL as key?

@@ -53,6 +56,10 @@ public Method getMethod() {
return method;
}

public ConcurrentMap<String, Object> getAttributeMap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning the same copy of attributeMap may lead to programatic mistake as many different java file may modify it, we may not have the track of who is modifying what what modifying.

Should we not have a put and get method of attribute map it self in this class. What do you say?


private final ConcurrentMap<String, Object> attributeMap = new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

private final Map<String, Object> attributeMap = new ConcurrentHashMap<>(); would be better.

public String getServiceName() {
return serviceName;
public ConcurrentMap<String, Object> getAttributeMap() {
return attributeMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

return the mutable member variable ** attributeMap** may be harmful as any caller can have the reference and modify it. may be we can provide put and get method for attributeMap in this class it self.

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

Successfully merging this pull request may close these issues.

None yet

4 participants