You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@eventmesh.apache.org by GitBox <gi...@apache.org> on 2022/06/15 11:08:17 UTC
[GitHub] [incubator-eventmesh] JellyBo opened a new pull request, #921: EM-Webhook: repair exception handling & optimize webHookConfig CURD logic
JellyBo opened a new pull request, #921:
URL: https://github.com/apache/incubator-eventmesh/pull/921
### Modifications
1. Repair exception handling
2. Optimize webHookConfig CURD logic
3. Add FileLock when writing webhookConfig to file
--
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.
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
[GitHub] [incubator-eventmesh] ruanwenjun commented on a diff in pull request #921: [ISSUE #865] EM-Webhook: repair exception handling & optimize webHookConfig CURD logic
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #921:
URL: https://github.com/apache/incubator-eventmesh/pull/921#discussion_r899984368
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -49,77 +49,54 @@ public FileWebHookConfigOperation(String filePath) throws FileNotFoundException
@Override
public Integer insertWebHookConfig(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File manuDir = new File(manuDirPath);
+ File manuDir = new File(getWebhookConfigManuDir(webHookConfig));
if (!manuDir.exists()) {
manuDir.mkdir();
}
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is existed");
- return 0;
- }
- try (FileWriter fw = new FileWriter(webhookConfigFile); BufferedWriter bw = new BufferedWriter(fw)) {
- bw.write(JsonUtils.serialize(webHookConfig));
- } catch (IOException e) {
- e.printStackTrace();
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is existed");
Review Comment:
```suggestion
logger.error("webhookConfig {} is already existed", webHookConfig.getCallbackPath());
```
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -49,77 +49,54 @@ public FileWebHookConfigOperation(String filePath) throws FileNotFoundException
@Override
public Integer insertWebHookConfig(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File manuDir = new File(manuDirPath);
+ File manuDir = new File(getWebhookConfigManuDir(webHookConfig));
if (!manuDir.exists()) {
manuDir.mkdir();
}
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is existed");
- return 0;
- }
- try (FileWriter fw = new FileWriter(webhookConfigFile); BufferedWriter bw = new BufferedWriter(fw)) {
- bw.write(JsonUtils.serialize(webHookConfig));
- } catch (IOException e) {
- e.printStackTrace();
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is existed");
return 0;
}
- return 1;
+ return writeToFile(webhookConfigFile, webHookConfig) ? 1 : 0;
}
@Override
public Integer updateWebHookConfig(WebHookConfig webHookConfig) {
Review Comment:
Please change this method to return a boolean value.
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -49,77 +49,54 @@ public FileWebHookConfigOperation(String filePath) throws FileNotFoundException
@Override
public Integer insertWebHookConfig(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File manuDir = new File(manuDirPath);
+ File manuDir = new File(getWebhookConfigManuDir(webHookConfig));
if (!manuDir.exists()) {
manuDir.mkdir();
}
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is existed");
- return 0;
- }
- try (FileWriter fw = new FileWriter(webhookConfigFile); BufferedWriter bw = new BufferedWriter(fw)) {
- bw.write(JsonUtils.serialize(webHookConfig));
- } catch (IOException e) {
- e.printStackTrace();
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is existed");
return 0;
}
- return 1;
+ return writeToFile(webhookConfigFile, webHookConfig) ? 1 : 0;
}
@Override
public Integer updateWebHookConfig(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (!webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is not existed");
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is not existed");
Review Comment:
```suggestion
logger.error("webhookConfig {} is not existed", webHookConfig.getCallbackPath());
```
##########
eventmesh-webhook/eventmesh-webhook-receive/src/main/java/org/apache/eventmesh/webhook/receive/storage/HookConfigOperationManage.java:
##########
@@ -97,10 +94,10 @@ public WebHookConfig queryWebHookConfigById(WebHookConfig webHookConfig) {
if(filePattern) return cacheWebHookConfig.get(webHookConfig.getCallbackPath());
else{
try {
- String content = configService.getConfig(webHookConfig.getManufacturerEventName() + DATA_ID_EXTENSION, GROUP_PREFIX + webHookConfig.getManufacturerName(), TIMEOUT_MS);
+ String content = configService.getConfig(MD5Utils.md5Hex(webHookConfig.getCallbackPath(), "UTF_8") + WebHookOperationConstant.DATA_ID_EXTENSION, WebHookOperationConstant.GROUP_PREFIX + webHookConfig.getManufacturerName(), WebHookOperationConstant.TIMEOUT_MS);
Review Comment:
Can we add a utils method to do this convert? If we change the path parten, we need to modified evenywhere.
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -49,77 +49,54 @@ public FileWebHookConfigOperation(String filePath) throws FileNotFoundException
@Override
public Integer insertWebHookConfig(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File manuDir = new File(manuDirPath);
+ File manuDir = new File(getWebhookConfigManuDir(webHookConfig));
if (!manuDir.exists()) {
manuDir.mkdir();
}
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is existed");
- return 0;
- }
- try (FileWriter fw = new FileWriter(webhookConfigFile); BufferedWriter bw = new BufferedWriter(fw)) {
- bw.write(JsonUtils.serialize(webHookConfig));
- } catch (IOException e) {
- e.printStackTrace();
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is existed");
return 0;
}
- return 1;
+ return writeToFile(webhookConfigFile, webHookConfig) ? 1 : 0;
}
@Override
public Integer updateWebHookConfig(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (!webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is not existed");
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is not existed");
return 0;
}
- try (FileWriter fw = new FileWriter(webhookConfigFile); BufferedWriter bw = new BufferedWriter(fw)) {
- bw.write(JsonUtils.serialize(webHookConfig));
- } catch (IOException e) {
- e.printStackTrace();
- return 0;
- }
- return 1;
+ return writeToFile(webhookConfigFile, webHookConfig) ? 1 : 0;
}
@Override
public Integer deleteWebHookConfig(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (!webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is not existed");
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is not existed");
return 0;
}
return webhookConfigFile.delete() ? 1 : 0;
}
@Override
public WebHookConfig queryWebHookConfigById(WebHookConfig webHookConfig) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File webhookConfigFile = new File(manuDirPath + FILE_SEPARATOR + webHookConfig.getManufacturerEventName() + FILE_EXTENSION);
+ File webhookConfigFile = new File(getWebhookConfigFilePath(webHookConfig));
if (!webhookConfigFile.exists()) {
- logger.error("webhookConfig " + manufacturerName + "_" + webHookConfig.getManufacturerEventName() + " is not existed");
+ logger.error("webhookConfig " + webHookConfig.getCallbackPath() + " is not existed");
return null;
}
-
return getWebHookConfigFromFile(webhookConfigFile);
}
@Override
public List<WebHookConfig> queryWebHookConfigByManufacturer(WebHookConfig webHookConfig, Integer pageNum,
Integer pageSize) {
- String manufacturerName = webHookConfig.getManufacturerName();
- String manuDirPath = filePath + FILE_SEPARATOR + manufacturerName;
- File manuDir = new File(manuDirPath);
+ File manuDir = new File(getWebhookConfigManuDir(webHookConfig));
Review Comment:
You don't need to call `getWebhookConfigManuDir` again
```suggestion
String webHookFilePath = getWebhookConfigManuDir(webHookConfig);
File manuDir = new File(webHookFilePath);
```
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -17,24 +17,24 @@
package org.apache.eventmesh.webhook.admin;
import java.io.*;
+import java.nio.channels.FileLock;
import java.util.ArrayList;
import java.util.List;
+import com.alibaba.nacos.common.utils.MD5Utils;
import org.apache.eventmesh.common.utils.JsonUtils;
import org.apache.eventmesh.webhook.api.WebHookConfig;
import org.apache.eventmesh.webhook.api.WebHookConfigOperation;
+import org.apache.eventmesh.webhook.api.WebHookOperationConstant;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class FileWebHookConfigOperation implements WebHookConfigOperation {
private static final Logger logger = LoggerFactory.getLogger(FileWebHookConfigOperation.class);
- private String filePath;
+ private final String filePath;
Review Comment:
```suggestion
private final String webHookFileDir;
```
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/NacosWebHookConfigOperation.java:
##########
@@ -68,49 +58,41 @@ public Integer insertWebHookConfig(WebHookConfig webHookConfig) {
String manufacturerName = webHookConfig.getManufacturerName();
try {
// 判断配置是否已存在
- if (configService.getConfig(webHookConfig.getManufacturerEventName() + DATA_ID_EXTENSION, GROUP_PREFIX + manufacturerName, TIMEOUT_MS) != null) {
- logger.error("insertWebHookConfig failed, config is existed");
+ if (configService.getConfig(getWebHookConfigDataId(webHookConfig), getManuGroupId(webHookConfig), TIMEOUT_MS) != null) {
Review Comment:
Please remove the chinese comment.
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -140,12 +117,50 @@ private WebHookConfig getWebHookConfigFromFile(File webhookConfigFile) {
while ((line = br.readLine()) != null) {
fileContent += line;
}
- } catch (FileNotFoundException e) {
- e.printStackTrace();
} catch (IOException e) {
- e.printStackTrace();
+ logger.error("get webhook from file {} error", webhookConfigFile.getPath(), e);
}
return JsonUtils.deserialize(fileContent, WebHookConfig.class);
}
+ private synchronized boolean writeToFile(File webhookConfigFile, WebHookConfig webHookConfig) {
+ FileOutputStream fos = null;
+ BufferedWriter bw = null;
+ FileLock lock = null;
Review Comment:
Please use try with resource, this can help you to close the resource in a safe way, of you need to add multiple try catch in your finally block.
```java
try{
if (fos != null) {
fos.close();
}
}catch (IOException e){
}
try{
if (bw != null) {
bw.close();
}
}catch (IOException e){
}
try{
if (lock != null) {
lock.close();
}
}catch (IOException e){
}
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -140,12 +117,50 @@ private WebHookConfig getWebHookConfigFromFile(File webhookConfigFile) {
while ((line = br.readLine()) != null) {
fileContent += line;
}
- } catch (FileNotFoundException e) {
- e.printStackTrace();
} catch (IOException e) {
- e.printStackTrace();
+ logger.error("get webhook from file {} error", webhookConfigFile.getPath(), e);
}
return JsonUtils.deserialize(fileContent, WebHookConfig.class);
}
+ private synchronized boolean writeToFile(File webhookConfigFile, WebHookConfig webHookConfig) {
+ FileOutputStream fos = null;
+ BufferedWriter bw = null;
+ FileLock lock = null;
+ try {
+ fos = new FileOutputStream(webhookConfigFile);
+ bw = new BufferedWriter(new OutputStreamWriter(fos));
+ // lock this file
+ lock = fos.getChannel().lock();
+ bw.write(JsonUtils.serialize(webHookConfig));
+ } catch (IOException e) {
+ logger.error("write webhookConfig {} to file error", webHookConfig.getCallbackPath());
+ return false;
+ } finally {
+ try {
+ if (fos != null) {
+ fos.close();
+ }
+ if (bw != null) {
+ bw.close();
+ }
+ if (lock != null) {
+ lock.release();
+ }
+ } catch (IOException e) {
+ logger.warn("writeToFile finally caught an exception", e);
+ }
+ }
+ return true;
+ }
+
+ private String getWebhookConfigManuDir(WebHookConfig webHookConfig) {
+ return filePath + WebHookOperationConstant.FILE_SEPARATOR + webHookConfig.getCloudEventName();
+ }
+
+ private String getWebhookConfigFilePath(WebHookConfig webHookConfig) {
+ return this.getWebhookConfigManuDir(webHookConfig) + WebHookOperationConstant.FILE_SEPARATOR + MD5Utils.md5Hex(webHookConfig.getCallbackPath(), "UTF_8") + WebHookOperationConstant.FILE_EXTENSION;
Review Comment:
Why we need to use `MD5Utils.md5Hex(webHookConfig.getCallbackPath(), "UTF_8")` to deal with the path here?
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -140,12 +117,50 @@ private WebHookConfig getWebHookConfigFromFile(File webhookConfigFile) {
while ((line = br.readLine()) != null) {
fileContent += line;
}
- } catch (FileNotFoundException e) {
- e.printStackTrace();
} catch (IOException e) {
- e.printStackTrace();
+ logger.error("get webhook from file {} error", webhookConfigFile.getPath(), e);
}
return JsonUtils.deserialize(fileContent, WebHookConfig.class);
}
+ private synchronized boolean writeToFile(File webhookConfigFile, WebHookConfig webHookConfig) {
Review Comment:
There is no need to add synchronized here? Will this method modify any variable?
##########
eventmesh-webhook/eventmesh-webhook-api/src/main/java/org/apache/eventmesh/webhook/api/WebHookOperationConstant.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.eventmesh.webhook.api;
+
+import java.io.File;
+
+/**
+ * Webhook常量类
+ */
+public class WebHookOperationConstant {
+
+ public static final String FILE_SEPARATOR = File.separator;
+
+ public static final String FILE_EXTENSION = ".json";
+
+ public static final String GROUP_PREFIX = "webhook_" ;
+
+ public static final String DATA_ID_EXTENSION = ".json";
+
+ public static final String MANUFACTURERS_DATA_ID = "manufacturers" + DATA_ID_EXTENSION;
+
+ public static final Integer TIMEOUT_MS = 3*1000;
Review Comment:
Use int here will cause extra implicit conversion.
```suggestion
public static final long TIMEOUT_MS = 3 * 1000L;
```
--
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.
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
[GitHub] [incubator-eventmesh] xwm1992 merged pull request #921: [ISSUE #865] EM-Webhook: repair exception handling & optimize webHookConfig CURD logic
Posted by GitBox <gi...@apache.org>.
xwm1992 merged PR #921:
URL: https://github.com/apache/incubator-eventmesh/pull/921
--
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.
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
[GitHub] [incubator-eventmesh] JellyBo commented on a diff in pull request #921: [ISSUE #865] EM-Webhook: repair exception handling & optimize webHookConfig CURD logic
Posted by GitBox <gi...@apache.org>.
JellyBo commented on code in PR #921:
URL: https://github.com/apache/incubator-eventmesh/pull/921#discussion_r900727681
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -140,12 +117,50 @@ private WebHookConfig getWebHookConfigFromFile(File webhookConfigFile) {
while ((line = br.readLine()) != null) {
fileContent += line;
}
- } catch (FileNotFoundException e) {
- e.printStackTrace();
} catch (IOException e) {
- e.printStackTrace();
+ logger.error("get webhook from file {} error", webhookConfigFile.getPath(), e);
}
return JsonUtils.deserialize(fileContent, WebHookConfig.class);
}
+ private synchronized boolean writeToFile(File webhookConfigFile, WebHookConfig webHookConfig) {
+ FileOutputStream fos = null;
+ BufferedWriter bw = null;
+ FileLock lock = null;
+ try {
+ fos = new FileOutputStream(webhookConfigFile);
+ bw = new BufferedWriter(new OutputStreamWriter(fos));
+ // lock this file
+ lock = fos.getChannel().lock();
+ bw.write(JsonUtils.serialize(webHookConfig));
+ } catch (IOException e) {
+ logger.error("write webhookConfig {} to file error", webHookConfig.getCallbackPath());
+ return false;
+ } finally {
+ try {
+ if (fos != null) {
+ fos.close();
+ }
+ if (bw != null) {
+ bw.close();
+ }
+ if (lock != null) {
+ lock.release();
+ }
+ } catch (IOException e) {
+ logger.warn("writeToFile finally caught an exception", e);
+ }
+ }
+ return true;
+ }
+
+ private String getWebhookConfigManuDir(WebHookConfig webHookConfig) {
+ return filePath + WebHookOperationConstant.FILE_SEPARATOR + webHookConfig.getCloudEventName();
+ }
+
+ private String getWebhookConfigFilePath(WebHookConfig webHookConfig) {
+ return this.getWebhookConfigManuDir(webHookConfig) + WebHookOperationConstant.FILE_SEPARATOR + MD5Utils.md5Hex(webHookConfig.getCallbackPath(), "UTF_8") + WebHookOperationConstant.FILE_EXTENSION;
Review Comment:
because the path may contain some speacial char like '/', which is illegal as a file name. I've changed to use URLEncoder.encode method.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
[GitHub] [incubator-eventmesh] Alonexc commented on pull request #921: EM-Webhook: repair exception handling & optimize webHookConfig CURD logic
Posted by GitBox <gi...@apache.org>.
Alonexc commented on PR #921:
URL: https://github.com/apache/incubator-eventmesh/pull/921#issuecomment-1157142555
Please check the license and make an issue associated with this pr.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
[GitHub] [incubator-eventmesh] JellyBo commented on pull request #921: [ISSUE #865] EM-Webhook: repair exception handling & optimize webHookConfig CURD logic
Posted by GitBox <gi...@apache.org>.
JellyBo commented on PR #921:
URL: https://github.com/apache/incubator-eventmesh/pull/921#issuecomment-1159395983
> I didn't see the whole code, just add some comment, please add license header and remove the chinese comment.
I gained a lot from your comments, thanks for your instruction!
--
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.
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
[GitHub] [incubator-eventmesh] ruanwenjun commented on a diff in pull request #921: [ISSUE #865] EM-Webhook: repair exception handling & optimize webHookConfig CURD logic
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on code in PR #921:
URL: https://github.com/apache/incubator-eventmesh/pull/921#discussion_r900845159
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -140,12 +117,50 @@ private WebHookConfig getWebHookConfigFromFile(File webhookConfigFile) {
while ((line = br.readLine()) != null) {
fileContent += line;
}
- } catch (FileNotFoundException e) {
- e.printStackTrace();
} catch (IOException e) {
- e.printStackTrace();
+ logger.error("get webhook from file {} error", webhookConfigFile.getPath(), e);
}
return JsonUtils.deserialize(fileContent, WebHookConfig.class);
}
+ private synchronized boolean writeToFile(File webhookConfigFile, WebHookConfig webHookConfig) {
+ FileOutputStream fos = null;
+ BufferedWriter bw = null;
+ FileLock lock = null;
+ try {
+ fos = new FileOutputStream(webhookConfigFile);
+ bw = new BufferedWriter(new OutputStreamWriter(fos));
+ // lock this file
+ lock = fos.getChannel().lock();
+ bw.write(JsonUtils.serialize(webHookConfig));
+ } catch (IOException e) {
+ logger.error("write webhookConfig {} to file error", webHookConfig.getCallbackPath());
+ return false;
+ } finally {
+ try {
+ if (fos != null) {
+ fos.close();
+ }
+ if (bw != null) {
+ bw.close();
+ }
+ if (lock != null) {
+ lock.release();
+ }
+ } catch (IOException e) {
+ logger.warn("writeToFile finally caught an exception", e);
+ }
+ }
+ return true;
+ }
+
+ private String getWebhookConfigManuDir(WebHookConfig webHookConfig) {
+ return filePath + WebHookOperationConstant.FILE_SEPARATOR + webHookConfig.getCloudEventName();
+ }
+
+ private String getWebhookConfigFilePath(WebHookConfig webHookConfig) {
+ return this.getWebhookConfigManuDir(webHookConfig) + WebHookOperationConstant.FILE_SEPARATOR + MD5Utils.md5Hex(webHookConfig.getCallbackPath(), "UTF_8") + WebHookOperationConstant.FILE_EXTENSION;
Review Comment:
OK, if so, it's better to add comment in the method.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org