[
https://issues.apache.org/jira/browse/EAGLE-81?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15141402#comment-15141402
]
ASF GitHub Bot commented on EAGLE-81:
-------------------------------------
Github user yonzhang commented on the pull request:
https://github.com/apache/incubator-eagle/pull/84#issuecomment-182518082
I applied the patch and run and found many issues with code structure,
error handling, existing code compatibility etc, let us hold off this patch and
let me try to reconstruct the code.
The main issues are:
1. Bad error handling. Exceptions are not brought from bottom to up so you
have no idea of what happens if some exception is thrown
2. No support for existing code compatibility. If we use
notificationexecutor, why do we keep AlertPersistExecutor? We should make sure
our code is complete. That is a bad idea to keep both old and new code.
3. Unit test is not complete, some not passed
4. Some basic java code issues for example
List<AlertDefinitionAPIEntity> activeAlerts = new
ArrayList<AlertDefinitionAPIEntity>();, but it is assigned immediately after
this. Some implementation class should be better in different package with good
naming convention.
> Notification Plugin Framework
> -----------------------------
>
> Key: EAGLE-81
> URL: https://issues.apache.org/jira/browse/EAGLE-81
> Project: Eagle
> Issue Type: New Feature
> Affects Versions: 0.3.0
> Reporter: Senthilkumar
> Assignee: Senthilkumar
> Original Estimate: 336h
> Remaining Estimate: 336h
>
> Today notification is only allowed for email .. We need more notification
> methods to send alert to external system .
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
