You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/02/04 15:10:40 UTC

[GitHub] [skywalking] kezhenxu94 opened a new pull request #4318: Reduce footprint via prototype pattern when tagging spans

kezhenxu94 opened a new pull request #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318
 
 
   ### Motivation:
   
   Reduce footprint when tagging spans with the deprecated API: `AbstractSpan#tag(String, String)`.
   
   ### Modifications:
   
   - Adopt [prototype pattern](https://en.wikipedia.org/wiki/Prototype_pattern) to create `Tag`, prevent from creating too many `StringTag` instances and `GC`ed, because ``AbstractSpan#tag(String, String)` simply `new` a `StringTag` every time and calls `AbstractSpan#tag(AbstractTag, String)`.
   
   - Replace `AbstractSpan#tag(String, String)` with `AbstractSpan#tag(AbstractTag, String)`.
   
   - Remove deprecated API: `AbstractSpan#tag(String, String)`.
   
   ### Result:
   
   - Deprecated API is removed.
   
   - Footprint is reduced.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on issue #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on issue #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318#issuecomment-582230371
 
 
   @JohnNiang This is not a decision, suggest taking the discussion to the mail list.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 merged pull request #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

kezhenxu94 merged pull request #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on issue #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on issue #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318#issuecomment-581966104
 
 
   BTW, we should add a new check rule, `don't use Deprecated API`.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on issue #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on issue #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318#issuecomment-581964295
 
 
   I know as a major upgrade, we could remove some, but basically, it is not necessary. So I want to keep them. 
   
   > yet I can rewrite the deprecated API to use the new prototype tag creation, for compatibility
   
   Yes, I want to keep that way, as it seems like it doesn't harm performance much.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] JohnNiang commented on issue #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

JohnNiang commented on issue #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318#issuecomment-582244844
 
 
   > Adding a new check rule is to avoid "future users use that method again", so what's your suggestion?
   
   We can't avoid using deprecated method, but it's our duty to tell future users that method is deprecated.
   
   Just like recent rectifications of invoking rewrited method in deprecated method body, but keep `Deprecated` annotation on the deprecated method.
   
   e.g.
   
   ```java
   @Deprecated // Keep this annotation
   public T deprecatedMethod() {
       return newMethod();
   }
   
   public T rewritedMethod() {
       // Latest codes here...
   }
   ```
   
   In a short, Using `Deprecated` api is my suggestion.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] JohnNiang commented on issue #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

JohnNiang commented on issue #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318#issuecomment-582227248
 
 
   > BTW, we should add a new check rule, `don't use Deprecated API`.
   
   I don't think this is a good decision. `Deprecated` annotation warns future users not to use that method again but new api instead.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on issue #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on issue #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318#issuecomment-581961605
 
 
   > Removing `AbstractSpan#tag(String, String)` is an incompatible change. I don't know, but assuming many, it is widely used out of our open source repo. Such as https://github.com/SkyAPM/java-plugin-extensions or private plugins?
   
   Well, if it’s marked as deprecated, I assume it is to be removed some day, and the major version 7.x is a good time IMO, yet I can rewrite the deprecated API to use the new prototype tag creation, for compatibility, which one do you prefer :)

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on issue #4318: Reduce footprint via prototype pattern when tagging spans

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on issue #4318: Reduce footprint via prototype pattern when tagging spans
URL: https://github.com/apache/skywalking/pull/4318#issuecomment-582232541
 
 
   > > BTW, we should add a new check rule, `don't use Deprecated API`.
   > 
   > I don't think this is a good decision. `Deprecated` annotation warns future users not to use that method again but new api instead.
   
   Adding a new check rule is to avoid "future users use that method again", so what's your suggestion?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services