kylin
kylin copied to clipboard
apache
[KYLIN-4780] Support dingtalk notify
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to Kylin?
Put an x in the boxes that apply
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation Update (if none of the other choices apply)
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
- [ ] I have create an issue on Kylin's jira, and have described the bug/feature there in detail
- [ ] Commit messages in my PR start with the related jira ID, like "KYLIN-0000 Make Kylin project open-source"
- [ ] Compiling and unit tests pass locally with my changes
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] If this change need a document change, I will prepare another pr against the
documentbranch - [ ] Any dependent changes have been merged
Further comments
If this is a relatively large or complex change, kick off the discussion at user@kylin or dev@kylin by explaining why you chose the solution you did and what alternatives you considered, etc...
This pull request introduces 4 alerts when merging cf374d61f396d953b6eb1317dd8476ec708f5bf0 into 9d7ee863304220c5fb53a4c88cb798ed3edb0891 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
- 1 for Boxed variable is never null
![lgtm-com[bot] avatar](https://avatars.githubusercontent.com/in/17324?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request introduces 4 alerts when merging 2deeb026968a68c87a998b72c0dffad29bb1d69e into 9d7ee863304220c5fb53a4c88cb798ed3edb0891 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
- 1 for Boxed variable is never null
This pull request introduces 4 alerts when merging 6ba193cb466e6ce8616ff9b4bfb24d16d6fd6779 into 9d7ee863304220c5fb53a4c88cb798ed3edb0891 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
- 1 for Boxed variable is never null
Pull Request Test Coverage Report for Build 7065
- 57 of 333 (17.12%) changed or added relevant lines in 20 files are covered.
- 35 unchanged lines in 10 files lost coverage.
- Overall coverage decreased (-0.02%) to 28.073%
| Totals | |
|---|---|
| Change from base Build 7032: | -0.02% |
| Covered Lines: | 26752 |
| Relevant Lines: | 95296 |
💛 - Coveralls
This pull request introduces 3 alerts when merging 41f2c01d9b2e3669768e6660266ccf6308befeb5 into 9d7ee863304220c5fb53a4c88cb798ed3edb0891 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
This pull request introduces 3 alerts when merging 73cdd58ab4a02b43a7c8f20f2f8673d0cc9a0332 into 3f3f114400e49cf8bd9ec48c888b253c81994f79 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
@yanghua The modification has been completed with reference to your suggestion, please review when you are free
This pull request introduces 3 alerts when merging 6440cda759b05e2325da1b8d33f6e311a2168ca1 into 3f3f114400e49cf8bd9ec48c888b253c81994f79 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
Codecov Report
Merging #1596 (d84c139) into master (0250081) will decrease coverage by
0.10%. The diff coverage is10.01%.
@@ Coverage Diff @@
## master #1596 +/- ##
============================================
- Coverage 25.42% 25.32% -0.11%
- Complexity 6765 6774 +9
============================================
Files 1508 1516 +8
Lines 93918 94422 +504
Branches 13158 13233 +75
============================================
+ Hits 23877 23910 +33
- Misses 67662 68126 +464
- Partials 2379 2386 +7
| Impacted Files | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| .../java/org/apache/kylin/common/KylinConfigBase.java | 12.42% <0.00%> (-0.05%) |
51.00 <0.00> (ø) |
|
| ...rg/apache/kylin/common/notify/DingTalkService.java | 0.00% <0.00%> (ø) |
0.00 <0.00> (?) |
|
| ...va/org/apache/kylin/common/notify/MailService.java | 0.00% <0.00%> (ø) |
0.00 <0.00> (?) |
|
| ...pache/kylin/common/notify/NotificationContext.java | 0.00% <0.00%> (ø) |
0.00 <0.00> (?) |
|
| ...e/kylin/common/notify/NotificationTransmitter.java | 0.00% <0.00%> (ø) |
0.00 <0.00> (?) |
|
| .../apache/kylin/common/notify/NotifyServiceBase.java | 0.00% <0.00%> (ø) |
0.00 <0.00> (?) |
|
| ...ylin/common/notify/util/NotificationConstants.java | 0.00% <0.00%> (ø) |
0.00 <0.00> (?) |
|
| ...in/java/org/apache/kylin/cube/CubeDescManager.java | 32.41% <0.00%> (-1.16%) |
12.00 <0.00> (ø) |
|
| ...c/main/java/org/apache/kylin/cube/CubeManager.java | 35.83% <0.00%> (-0.81%) |
44.00 <0.00> (ø) |
|
| ...c/main/java/org/apache/kylin/cube/CubeSegment.java | 41.97% <0.00%> (-0.31%) |
65.00 <0.00> (ø) |
|
| ... and 94 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update ca08012...d84c139. Read the comment docs.
This pull request introduces 3 alerts when merging b2a10f441917e90192372fb8a68944879a23c070 into 49d7b9bd6c022bf842c731c0521f0898e16566bf - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
This pull request introduces 3 alerts when merging 439fb975937b9ab420cb66446f40f8b72758af23 into 5be135c61c80b9241ac7632dec41cc7209b9d23a - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
This pull request introduces 3 alerts when merging 6969865ac4a94673a0ab7486f3378514d44417ba into ca0801201a4a5b3a78dd6fac788cbc5bde7e2bbd - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
This pull request introduces 3 alerts when merging d84c139af8b97e6e360365bf3de1ade602300735 into 69368b08a770f6e54c4ecf43c538331e13cb03b2 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
This pull request introduces 3 alerts when merging 77656aa2edfe2c600c7a8df29983c33ca00b8a21 into 69368b08a770f6e54c4ecf43c538331e13cb03b2 - view on LGTM.com
new alerts:
- 3 for Dereferenced variable may be null
@zhangayqian Would you please review this PR?
Sorry for late. This PR changed a lot and there is new dependencies be introduced. We may need some time to discuss it.
I wish this patch could be well designed for extensible purpose, and a notification framework should be developed from my side. So, if other guys want to add support for We-chat or Slack, it should be very easy because he/her should only provided some implementation depend on this notification framework .
I found that you provided a simple framework with some base classes : NotifyServiceBase, NotificationContext, NotificationTransmitter. I am not really sure if it is enough extensible. What do you think ?
Besides, the alibaba-dingtalk-service-sdk is not maintained by apache software foundation or and other open source organization, and the source code didn't have a proper asf-compatible license at all.
Hello chenjie @chenjie-sau ,
Thanks for raising this PR!
I think this is a good feature, but for the implementation, it can be improved a little bit. Firstly, the dingding sdk's license is unknown, so it is not okya for Kylin to bundle it in the release. Secondly, the notification module can be designed as pluggable. For example, user can implement an interface and then config the implementation class in kylin.properties. Kylin then will load the class at runtime by name, and then send the notification.
Thank you for your valuable suggestions. Some suggestions are very reasonable. I will consider each suggestion for certification and redesign a better solution.