[
https://issues.apache.org/jira/browse/KYLIN-4780?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17310453#comment-17310453
]
ASF GitHub Bot commented on KYLIN-4780:
---------------------------------------
yanghua commented on a change in pull request #1596:
URL: https://github.com/apache/kylin/pull/1596#discussion_r603032595
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/DingTalkService.java
##########
@@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kylin.common.notify;
+
+import com.dingtalk.api.DefaultDingTalkClient;
+import com.dingtalk.api.DingTalkClient;
+import com.dingtalk.api.request.OapiRobotSendRequest;
+import com.dingtalk.api.response.OapiRobotSendResponse;
+import com.taobao.api.ApiException;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.kylin.common.notify.util.DingTalkNotificationUtil;
+import org.apache.kylin.common.notify.util.NotificationConstants;
+import org.apache.kylin.common.notify.util.SecretKeyUtil;
+import org.apache.kylin.common.util.Pair;
+import org.apache.kylin.common.util.StringUtil;
+import org.apache.kylin.shaded.com.google.common.base.Strings;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class DingTalkService extends NotifyServiceBase {
+ private static final org.slf4j.Logger logger =
LoggerFactory.getLogger(DingTalkService.class);
+
+ private static final Pattern TOKEN_REGEX =
Pattern.compile("(^[a-z0-9A-Z]*):?([a-z0-9A-Z]*)@?(.*)?");
+ private static final Pattern PHONE_REGEX =
Pattern.compile("1[3456789]\\d{9}");
+
+ private Boolean enabled = Boolean.TRUE;
+
+ private String url =
"https://oapi.dingtalk.com/robot/send?access_token=%s";;
+ private String secretParam = "×tamp=%s&sign=%s";
+
+ public DingTalkService(NotificationContext notificationContext) {
+ super(notificationContext);
+ this.enabled =
getNotificationContext().getConfig().isNotificationEnabled();
+ }
+
+ public boolean sendNotification() {
+ if (!enabled) {
+ logger.info("DingTalk service is disabled; this DingTalk will not
be delivered");
+ logger.info("To enable notify service, set
'kylin.job.notification-enabled=true' in kylin.properties");
+ return false;
+ }
+
+ if
(CollectionUtils.isEmpty(getNotificationContext().getReceivers().get(NotificationConstants.NOTIFY_DINGTALK_LIST)))
{
+ logger.warn("no need to send dingtalk, receivers is empty");
+ return false;
+ } else {
+ logger.info("prepare to send dingtalk to:{}",
getNotificationContext().getReceivers().get(NotificationConstants.NOTIFY_DINGTALK_LIST));
+ String title =
DingTalkNotificationUtil.getTitle(getNotificationContext().getContent().getFirst());
+ String contentDingTalk =
DingTalkNotificationUtil.getContent(getNotificationContext().getState(), title,
getNotificationContext().getContent().getSecond());
+ return
sendContent(getNotificationContext().getReceivers().get(NotificationConstants.NOTIFY_DINGTALK_LIST),
title, contentDingTalk);
+ }
+ }
+
+ private boolean sendContent(List<String> infos, String title, String
contentDingTalk) {
+ boolean res = false;
+ for (String dingTalkInfo : infos) {
+ if (StringUtil.isEmpty(dingTalkInfo)) {
+ logger.warn("dingtalk service is disabled; this notify will
not be delivered, Please check the information {}", dingTalkInfo);
Review comment:
wrong warning message?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/NotificationContext.java
##########
@@ -0,0 +1,130 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kylin.common.notify;
+
+import org.apache.kylin.common.KylinConfig;
+import org.apache.kylin.common.util.Pair;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * All basic information of the notification.
+ */
+public class NotificationContext {
+ private KylinConfig config;
+
+ /**
+ * Notification recipient，include email and dingTalk
+ */
+ private Map<String, List<String>> receivers;
+
+ /**
+ * state of job
+ */
+ private String state;
+
+ /**
+ * content of notification, include title and detail
+ */
+ private Pair<String[], Map<String, Object>> content;
+
+ /**
+ * Send in HTML
+ */
+ private boolean isHtmlMsg = true;
+
+ /**
+ * when isHtmlMsg is false, subject and info will be sended as text
+ */
+ private String subject = "";
+
+ private String info = "";
+
+ public NotificationContext(KylinConfig config, Map<String, List<String>>
receivers, String state, Pair<String[], Map<String, Object>> content) {
+ this.config = config;
+ this.receivers = receivers;
+ this.state = state;
+ this.content = content;
+ }
+
+ public NotificationContext(KylinConfig config, Map<String, List<String>>
receivers, String subject, String info, boolean isHtmlMsg) {
+ this.config = config;
+ this.receivers = receivers;
+ this.subject = subject;
+ this.info = info;
+ this.isHtmlMsg = isHtmlMsg;
+ }
+
+ public KylinConfig getConfig() {
+ return config;
+ }
+
+ public void setConfig(KylinConfig config) {
+ this.config = config;
+ }
+
+ public Map<String, List<String>> getReceivers() {
+ return receivers;
+ }
+
+ public void setReceivers(Map<String, List<String>> receivers) {
+ this.receivers = receivers;
+ }
+
+ public String getState() {
+ return state;
+ }
+
+ public void setState(String state) {
+ this.state = state;
+ }
+
+ public Pair<String[], Map<String, Object>> getContent() {
+ return content;
+ }
+
+ public void setContent(Pair<String[], Map<String, Object>> content) {
+ this.content = content;
+ }
+
+ public boolean isHtmlMsg() {
+ return isHtmlMsg;
+ }
+
+ public void setHtmlMsg(boolean htmlMsg) {
+ isHtmlMsg = htmlMsg;
+ }
+
+ public String getSubject() {
+ return subject;
+ }
+
+ public void setSubject(String subject) {
+ this.subject = subject;
+ }
+
+ public String getInfo() {
+ return info;
+ }
+
+ public void setInfo(String info) {
+ this.info = info;
+ }
+}
Review comment:
It would be better to override the `equals` and `hashcode` for a model
class.
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/MailService.java
##########
@@ -78,12 +73,17 @@ public boolean sendMail(List<String> receivers, String
subject, String content)
* @return true or false indicating whether the email was delivered
successfully
* @throws IOException
*/
- public boolean sendMail(List<String> receivers, String subject, String
content, boolean isHtmlMsg) {
+ private boolean sendMail(List<String> receivers, String subject, String
content, boolean isHtmlMsg) {
if (!enabled) {
logger.info("Email service is disabled; this mail will not be
delivered: " + subject);
logger.info("To enable mail service, set
'kylin.job.notification-enabled=true' in kylin.properties");
return false;
+ } else {
+ if (host.isEmpty()) {
Review comment:
Can we extract this if block? There is no need to have the `else` at
line No.82.
##########
File path:
core-common/src/test/java/org/apache/kylin/common/notify/DingTalkServiceTest.java
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+*/
+
+package org.apache.kylin.common.notify;
+
+import org.apache.kylin.common.KylinConfig;
+import org.apache.kylin.common.notify.util.NotificationConstants;
+import org.apache.kylin.common.util.LocalFileMetadataTestCase;
+import org.apache.kylin.common.util.Pair;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+@Ignore("convenient trial tool for dev")
+public class DingTalkServiceTest extends LocalFileMetadataTestCase {
+
+ @Before
+ public void setup() throws Exception {
+ this.createTestMetadata();
+ }
+
+ @After
+ public void after() throws Exception {
+ this.cleanupTestMetadata();
+ }
+
+ @Test
+ public void testSendDingTalk() throws IOException {
+ KylinConfig config = KylinConfig.getInstanceFromEnv();
+ List<String> receivers = new ArrayList<String>(1);
+
receivers.add("98a3994aecc22763b135b91e953fd32eec60e1bf10190f7af4eca77049c433e@13675555555");
+ Map<String, List<String>> receiversMap = new HashMap();
+ receiversMap.put(NotificationConstants.NOTIFY_DINGTALK_LIST,
receivers);
+
+ Map<String, Object> content = new HashMap();
+ content.put("info", "dingtalk notification");
+
+ Pair<String[], Map<String, Object>> mapPair = Pair.newPair(new
String[]{"test"}, content);
+ NotificationContext notificationInfo = new NotificationContext(config,
receiversMap, NotificationConstants.JOB_ERROR, mapPair);
+
+ DingTalkService dingTalkservice = new
DingTalkService(notificationInfo);
+ boolean sent = sendTestDingTalk(dingTalkservice);
+ assert !sent;
+
+ System.setProperty("kylin.job.notification-enabled", "false");
+ // set kylin.job.notification-enabled=false, and run again, this time
should be no mail delivered
+ dingTalkservice = new DingTalkService(notificationInfo);
+ sent = sendTestDingTalk(dingTalkservice);
Review comment:
just call `dingTalkService.sendNotification()` here?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/util/MailTemplateProvider.java
##########
@@ -36,8 +34,6 @@
*/
public class MailTemplateProvider {
Review comment:
ditto, we can merge this class and `MailNotificationUtil `.
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/NotificationTransmitter.java
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kylin.common.notify;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.kylin.common.notify.util.NotificationConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Entity class for notification sending.
+ */
+public class NotificationTransmitter {
+ private static final Logger logger =
LoggerFactory.getLogger(NotificationTransmitter.class);
+
+ private List<NotifyServiceBase> notifyServices = new ArrayList<>();
+
+ private ExecutorService pool = Executors.newCachedThreadPool();
+
+ private List<Future> futureList = new ArrayList<>();
+
+ public NotificationTransmitter(NotificationContext notificationInfo) {
Review comment:
Should this class be a singleton class?
##########
File path:
core-common/src/test/java/org/apache/kylin/common/notify/MailServiceTest.java
##########
@@ -44,25 +48,30 @@ public void after() throws Exception {
@Test
public void testSendEmail() throws IOException {
-
KylinConfig config = KylinConfig.getInstanceFromEnv();
+ List<String> receivers = new ArrayList<String>(1);
+ receivers.add("[email protected]");
+ Map<String, List<String>> receiversMap = new HashMap();
+ receiversMap.put(NotificationConstants.NOTIFY_EMAIL_LIST, receivers);
+
+ Map<String, Object> content = new HashMap();
+ content.put("info", "email notification");
+ Pair<String[], Map<String, Object>> mapPair = Pair.newPair(new
String[]{"test"}, content);
+
+ NotificationContext notificationInfo = new NotificationContext(config,
receiversMap, NotificationConstants.JOB_ERROR, mapPair);
- MailService mailservice = new MailService(config);
+ MailService mailservice = new MailService(notificationInfo);
boolean sent = sendTestEmail(mailservice);
- assert sent;
+ assert !sent;
System.setProperty("kylin.job.notification-enabled", "false");
// set kylin.job.notification-enabled=false, and run again, this time
should be no mail delivered
- mailservice = new MailService(config);
+ mailservice = new MailService(notificationInfo);
sent = sendTestEmail(mailservice);
Review comment:
just call `mailservice.sendNotification()`?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/util/DingTalkTemplateProvider.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kylin.common.notify.util;
+
+import org.apache.kylin.shaded.com.google.common.base.Strings;
+
+import java.util.Map;
+
+public class DingTalkTemplateProvider {
+
+ private static DingTalkTemplateProvider DEFAULT_INSTANCE = new
DingTalkTemplateProvider();
+
+ public static DingTalkTemplateProvider getInstance() {
+ return DEFAULT_INSTANCE;
+ }
+
+ public String buildDingTalkContent(String state, String title, Map<String,
Object> data) {
Review comment:
IMO, we can merge `DingTalkNotificationUtil ` and this class. WDYT?
--
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:
[email protected]
> Refactor AbstractExecutable to extract the notification relevant business
> logic into a single component
> -------------------------------------------------------------------------------------------------------
>
> Key: KYLIN-4780
> URL: https://issues.apache.org/jira/browse/KYLIN-4780
> Project: Kylin
> Issue Type: Improvement
> Components: Integration
> Reporter: vinoyang
> Assignee: chenjie
> Priority: Major
>
> Currently, Kylin supports mail service to know the change around the cube.
> However, there are some issues about the design, e.g.
> * AbstractExecutable contains an implementation of the mechanism of a
> notification listener, but only support mail service, it does not belong
> there;
> * The current implementation is hard to scale to other notification e.g.
> Ding Talk(which is a popular production supports by Alibaba)
> I propose to refactor the current implementation to extract the notification
> logic and redesign a new common notification/listener
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
