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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
