[
https://issues.apache.org/jira/browse/KYLIN-4780?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17300247#comment-17300247
]
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_r593102391
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/MailService.java
##########
@@ -134,4 +135,17 @@ public boolean sendMail(List<String> receivers, String
subject, String content,
return false;
}
}
+
+ public boolean sendNotification() {
+ if (receivers.get(NotificationConstant.NOTIFY_EMAIL_LIST) == null) {
+ logger.warn("no need to send email, content is null");
Review comment:
The **content** is null?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/MailService.java
##########
@@ -134,4 +135,17 @@ public boolean sendMail(List<String> receivers, String
subject, String content,
return false;
}
}
+
+ public boolean sendNotification() {
+ if (receivers.get(NotificationConstant.NOTIFY_EMAIL_LIST) == null) {
Review comment:
Can we judge if it's empty("")?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/MailService.java
##########
@@ -134,4 +135,17 @@ public boolean sendMail(List<String> receivers, String
subject, String content,
return false;
}
}
+
+ public boolean sendNotification() {
+ if (receivers.get(NotificationConstant.NOTIFY_EMAIL_LIST) == null) {
+ logger.warn("no need to send email, content is null");
+ return false;
+ } else {
+ logger.info("prepare to send email to:{}",
receivers.get(NotificationConstant.NOTIFY_EMAIL_LIST));
+ String contentEmail = MailNotificationUtil.getMailContent(state,
content.getSecond());
+ String title =
MailNotificationUtil.getMailTitle(content.getFirst());
+ return
sendMail(receivers.get(NotificationConstant.NOTIFY_EMAIL_LIST), title,
contentEmail);
Review comment:
So, can we change the access modifier of `sendMail` to be `private`?
##########
File path:
core-common/src/test/java/org/apache/kylin/common/notify/DingTalkServiceTest.java
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.LocalFileMetadataTestCase;
+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.List;
+
+@Ignore("convenient trial tool for dev")
+public class DingTalkServiceTest extends LocalFileMetadataTestCase {
+
+ @Before
+ public void setup() throws Exception {
+ this.createTestMetadata();
+
Review comment:
Why we need this empty line?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/NotifyService.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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 org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+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;
+import java.util.function.Consumer;
+
+
+public class NotifyService {
+ private static final Logger logger =
LoggerFactory.getLogger(NotifyService.class);
+
+ private NotifyServiceBase[] notifyServices;
+
+ private ExecutorService pool = Executors.newCachedThreadPool();
+
+ private List<Future> futureList = new ArrayList<>();
+
+ public NotifyService(KylinConfig config) {
+ notifyServices = new NotifyServiceBase[]{
+ new MailService(config),
+ new DingTalkService(config)
+ };
+ }
+
+ public boolean sendNotification(Map<String, List<String>> receivers,
String state, Pair<String[], Map<String, Object>> content) {
+ boolean res = Boolean.TRUE;
+
+ Consumer<NotifyServiceBase> action = new Consumer<NotifyServiceBase>()
{
+
+ @Override
+ public void accept(NotifyServiceBase notifyServiceBase) {
+ notifyServiceBase.sendNotificationInfo(receivers, state,
content);
+ }
+ };
+ Arrays.stream(notifyServices).forEach(action);
+
+ for (Callable callable : notifyServices) {
+ Future f = pool.submit(callable);
+ futureList.add(f);
+ }
+
+ for (Future f : futureList) {
+ try {
+ if (f.get(1,
TimeUnit.MINUTES).toString().equalsIgnoreCase("false")) {
+ res = false;
+ }
+ } catch (Exception e) {
+ res = false;
+ logger.error("push notification for email or dingtalk occurred
exception");
Review comment:
Can we log the detail of the exception?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/util/NotificationConstant.java
##########
@@ -0,0 +1,39 @@
+/*
+ * 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;
+
+public class NotificationConstant {
Review comment:
`NotificationConstants` looks better?
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/util/NotificationConstant.java
##########
@@ -0,0 +1,39 @@
+/*
+ * 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;
+
+public class NotificationConstant {
+ public static final String NOTIFY_EMAIL_LIST = "notify_email_list";
+ public static final String NOTIFY_DINGTALK_LIST = "notify_dingtalk_list";
+
+ public static final String ERROR = "ERROR";
Review comment:
Does it mean the status or result of sending? If yes, can we rename it
to `STATUS_ERROR`?
##########
File path:
core-common/src/test/java/org/apache/kylin/common/notify/DingTalkServiceTest.java
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.LocalFileMetadataTestCase;
+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.List;
+
+@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 testSendEmail() throws IOException {
+
Review comment:
ditto
##########
File path:
core-common/src/test/java/org/apache/kylin/common/notify/DingTalkServiceTest.java
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.LocalFileMetadataTestCase;
+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.List;
+
+@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 testSendEmail() throws IOException {
+
+ KylinConfig config = KylinConfig.getInstanceFromEnv();
+
+ DingTalkService dingTalkservice = new DingTalkService(config);
+ boolean sent = sendTestEmail(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(config);
+ sent = sendTestEmail(dingTalkservice);
+ assert !sent;
+
Review comment:
ditto
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/util/SecretKeyUtil.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.commons.codec.binary.Base64;
+import org.apache.kylin.common.util.Pair;
+import org.slf4j.LoggerFactory;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+
+public class SecretKeyUtil {
+ private static final org.slf4j.Logger logger =
LoggerFactory.getLogger(SecretKeyUtil.class);
+
+ public static Pair<Long, String> createSecretKey(String secret) {
+ long timestamp = System.currentTimeMillis();
+
+ String stringToSign = timestamp + "\n" + secret;
+ try {
+ Mac mac = Mac.getInstance("HmacSHA256");
+ mac.init(new
SecretKeySpec(secret.getBytes(StandardCharsets.UTF_8), "HmacSHA256"));
+ byte[] signData =
mac.doFinal(stringToSign.getBytes(StandardCharsets.UTF_8));
+ String sign = URLEncoder.encode(new
String(Base64.encodeBase64(signData), StandardCharsets.UTF_8), "UTF-8");
+ return Pair.newPair(timestamp, sign);
+ } catch (Exception e) {
+ logger.error("create mac of dingtalk occurred error, please
check.");
Review comment:
We can give users more detail about the exception.
##########
File path:
core-common/src/main/java/org/apache/kylin/common/notify/NotifyServiceBase.java
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.util.Pair;
+
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Callable;
+
+public abstract class NotifyServiceBase implements Callable<Boolean> {
+
+ public Map<String, List<String>> receivers;
+ public String state;
+ public Pair<String[], Map<String, Object>> content;
+
+ public abstract boolean sendNotification();
Review comment:
This design is not very graceful. IMO, we can define a
NotificationContext Model to box these details and the context will be the
parameter of the constructor. Then, there is not necessary to add the next
method, right?
##########
File path:
core-common/src/test/java/org/apache/kylin/common/notify/DingTalkServiceTest.java
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.LocalFileMetadataTestCase;
+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.List;
+
+@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 testSendEmail() throws IOException {
+
+ KylinConfig config = KylinConfig.getInstanceFromEnv();
+
+ DingTalkService dingTalkservice = new DingTalkService(config);
+ boolean sent = sendTestEmail(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(config);
+ sent = sendTestEmail(dingTalkservice);
+ assert !sent;
+
+ }
+
+ private boolean sendTestEmail(DingTalkService dingTalkService) {
+
Review comment:
ditto
----------------------------------------------------------------
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)
