You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/03/10 02:13:39 UTC

[GitHub] [skywalking] wu-sheng opened a new pull request #4475: Add file change detection mechanism

wu-sheng opened a new pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475
 
 
   Due to some strict management rules inside the enterprise, some companies use Vault to manage username and password of the storage. 
   
   This PR makes the first change, it provides the basic file content(or we should say modified flag) changed detection mechanism. Once it is changed, the notification will be triggered.
   
   The next step will be activating this mechanism for ES storage. Once username and password changed, the ES client should trigger reconnection.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] hanahmily commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

hanahmily commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390074768
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   > I think your idea is only coming from the cert file, as you don't need to read it, right? For ES case, this is a double read or extra memory case.
   
   No. For ES and cert file, double read or extra memory is necessary. Because file contents change leads to a total reconnection. For cert file, all incoming gRPC connections should close as ES does. SSL file notifier also loads the binary into heap to compare the content to avoid unnecessary reconnection.
   
   And I think we should provide an abstract class to implement notifier interface, which can do binary comparison. If anyone needs this feature can extend it instead of implementing notifier.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390083996
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
+ * fetch the new content of the file and check with the prev one. If content changed, it will notify the listener.
+ *
+ * File Change
+ */
+@RequiredArgsConstructor
+@Slf4j
+public class FileChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    /**
+     * The list contains all monitors.
+     */
+    private static List<FileChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The absolute path of the monitored file.
+     */
+    private final String filePath;
+    /**
+     * Trigger notification when file is not there.
+     */
+    private final boolean acceptFileNotExisting;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    /**
+     * The notifier when file content changed.
+     */
+    private final ContentChangedNotifier notifier;
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The last modify time of the {@link #filePath}
+     */
+    private long lastModifiedTimestamp = 0;
+
+    /**
+     * Start the file monitor for this instance.
+     */
+    public synchronized void start() {
+        if (FILE_MONITOR_TASK_SCHEDULER == null) {
+            FILE_MONITOR_TASK_SCHEDULER = Executors.newSingleThreadScheduledExecutor()
+                                                   .scheduleAtFixedRate(
+                                                       FileChangeMonitor::run, 1, 1,
+                                                       TimeUnit.SECONDS
+                                                   );
+        }
+
+        this.checkAndNotify();
+        MONITOR_INSTANCES.add(this);
+    }
+
+    public synchronized void stop() {
+        MONITOR_INSTANCES.remove(this);
+    }
+
+    /**
+     * Check the file status, if changed, send the notification.
+     */
+    private void checkAndNotify() {
+        if (System.currentTimeMillis() - lastCheckTimestamp < watchingPeriodInSec * 1000) {
+            // Don't reach the period threshold, ignore this check.
+            return;
+        }
+        File targetFile = new File(filePath);
+        if (!targetFile.exists() && acceptFileNotExisting) {
+            notifier.fileNotFound();
+        }
+        if (targetFile.isFile()) {
+            long lastModified = targetFile.lastModified();
+
+            if (lastModified != lastModifiedTimestamp) {
+                try (FileInputStream fileInputStream = new FileInputStream(targetFile)) {
+                    notifier.newFileContent(fileInputStream);
+                } catch (FileNotFoundException e) {
+                    log.error("The existed file turns to missing, watch file=" + filePath, e);
+                } catch (IOException e) {
+                    log.error("Read file failure, watch file=" + filePath, e);
+                } finally {
+                    lastModifiedTimestamp = lastModified;
+                }
+            }
+        }
 
 Review comment:
   At least, this part should be replaced, and you can provide a so called `Callback` to the caller

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on issue #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on issue #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#issuecomment-596958666
 
 
   According to the feedback, I will add file group monitoring as CA cert requires this too. Update later.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390149210
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,188 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
+ * fetch the new content of the file and check with the prev one. If content changed, it will notify the listener.
+ *
+ * File Change
+ */
+@RequiredArgsConstructor
+@Slf4j
+public class FileChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    /**
+     * The list contains all monitors.
+     */
+    private static List<FileChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The absolute path of the monitored file.
+     */
+    private final String filePath;
+    /**
+     * Trigger notification when file is not there.
+     */
+    private final boolean acceptFileNotExisting;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    /**
+     * The notifier when file content changed.
+     */
+    private final FileChangedNotifier notifier;
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The last modify time of the {@link #filePath}
+     */
+    private long lastModifiedTimestamp = 0;
+
+    /**
+     * Start the file monitor for this instance.
+     */
+    public synchronized void start() {
 
 Review comment:
   Will fix.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] codecov-io edited a comment on issue #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#issuecomment-596927662
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=h1) Report
   > Merging [#4475](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/73d0802cd69e7bdf5fd925060afc00207edb711d?src=pr&el=desc) will **increase** coverage by `0.08%`.
   > The diff coverage is `59.18%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4475/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4475      +/-   ##
   ==========================================
   + Coverage   24.95%   25.04%   +0.08%     
   ==========================================
     Files        1231     1232       +1     
     Lines       28496    28538      +42     
     Branches     3905     3913       +8     
   ==========================================
   + Hits         7111     7147      +36     
     Misses      20726    20726              
   - Partials      659      665       +6
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ap/server/core/storage/ttl/DataTTLKeeperTimer.java](https://codecov.io/gh/apache/skywalking/pull/4475/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvc3RvcmFnZS90dGwvRGF0YVRUTEtlZXBlclRpbWVyLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
   | [...ing/oap/server/library/util/FileChangeMonitor.java](https://codecov.io/gh/apache/skywalking/pull/4475/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItbGlicmFyeS9saWJyYXJ5LXV0aWwvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9saWJyYXJ5L3V0aWwvRmlsZUNoYW5nZU1vbml0b3IuamF2YQ==) | `67.44% <67.44%> (ø)` | |
   | [...walking/oap/server/core/analysis/Downsampling.java](https://codecov.io/gh/apache/skywalking/pull/4475/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvYW5hbHlzaXMvRG93bnNhbXBsaW5nLmphdmE=) | `100% <0%> (+100%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=footer). Last update [73d0802...4b064ce](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390083810
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   > What is `FileSystems.getDefault().newWatchService()` for? That doesn't look like a watcher with callback to me. Do I miss anything?
   
   Hope the code snippets helps, my concern is that the built-in tool is well optimized and tested, and reduce our maintenance work:
   
   ```java
       public static void main(String[] args) throws IOException, InterruptedException {
           final WatchService watchService = FileSystems.getDefault().newWatchService();
           final Path directoryToWatch = Paths.get("/tmp/directory-to-watch");
           directoryToWatch.register(watchService, StandardWatchEventKinds.ENTRY_MODIFY);
   
           for (final WatchKey readyKey = watchService.take(); readyKey.isValid(); readyKey.reset()) {
               for (final WatchEvent<?> event : readyKey.pollEvents()) {
                   assert event.kind().equals(StandardWatchEventKinds.ENTRY_MODIFY);
                   final Path path = (Path) event.context();
                   System.out.println("file is modified = " + directoryToWatch.resolve(path));
               }
           }
       }
   ```

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] hanahmily commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

hanahmily commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390062314
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   We should consider another scenario if the content of file doesn't change the monitor should not notify the related component.
   A possible approach is to compare the binary data of them.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng merged pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng merged pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475
 
 
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] hanahmily commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

hanahmily commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390070679
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   I prefer to leave it to the monitor since it extracts the input-stream from the target file. And the comparison is a common requirement for all of the notifiers, Do you have some scenario needs to get notification of modification without changing the content? if that exists, it's sane for it.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] dmsolr commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

dmsolr commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390693872
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java
 ##########
 @@ -0,0 +1,242 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * MultipleFilesChangeMonitor provides the capability to detect file or multiple files changed. It provide second level
+ * change detection and feedback mechanism.
+ *
+ * Due to memory cost, this monitor mechanism is not suitable for small files and usually being changed on the runtime
+ * by user manually or 3rd party tool. Typical, these files are config information or authentication files.
+ */
+@Slf4j
+public class MultipleFilesChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    private static ReentrantLock SCHEDULER_CHANGE_LOCK = new ReentrantLock();
+    /**
+     * The list contains all monitors.
+     */
+    private static List<MultipleFilesChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    private List<WatchedFile> watchedFiles;
+    private FilesChangedNotifier notifier;
+
+    /**
+     * Create a new monitor for the given files
+     *
+     * @param watchingPeriodInSec The check period.
+     * @param notifier            to accept the file changed notification.
+     * @param files               to be monitored.
+     */
+    public MultipleFilesChangeMonitor(long watchingPeriodInSec,
+                                      FilesChangedNotifier notifier,
+                                      String... files) {
+        watchedFiles = new ArrayList<>();
+        this.watchingPeriodInSec = watchingPeriodInSec;
+        this.notifier = notifier;
+        for (final String file : files) {
+            WatchedFile monitor = new WatchedFile(file);
+            watchedFiles.add(monitor);
+        }
+    }
+
+    /**
+     * Check file changed status, if so, send the notification.
+     */
+    private void checkAndNotify() {
+        if (System.currentTimeMillis() - lastCheckTimestamp < watchingPeriodInSec * 1000) {
+            // Don't reach the period threshold, ignore this check.
+            return;
+        }
 
 Review comment:
   I got it. Monitors own their private schedule, `watchingPeriodInSec`.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] dmsolr commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

dmsolr commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390682366
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java
 ##########
 @@ -0,0 +1,242 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * MultipleFilesChangeMonitor provides the capability to detect file or multiple files changed. It provide second level
+ * change detection and feedback mechanism.
+ *
+ * Due to memory cost, this monitor mechanism is not suitable for small files and usually being changed on the runtime
+ * by user manually or 3rd party tool. Typical, these files are config information or authentication files.
+ */
+@Slf4j
+public class MultipleFilesChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    private static ReentrantLock SCHEDULER_CHANGE_LOCK = new ReentrantLock();
+    /**
+     * The list contains all monitors.
+     */
+    private static List<MultipleFilesChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    private List<WatchedFile> watchedFiles;
+    private FilesChangedNotifier notifier;
+
+    /**
+     * Create a new monitor for the given files
+     *
+     * @param watchingPeriodInSec The check period.
+     * @param notifier            to accept the file changed notification.
+     * @param files               to be monitored.
+     */
+    public MultipleFilesChangeMonitor(long watchingPeriodInSec,
+                                      FilesChangedNotifier notifier,
+                                      String... files) {
+        watchedFiles = new ArrayList<>();
+        this.watchingPeriodInSec = watchingPeriodInSec;
+        this.notifier = notifier;
+        for (final String file : files) {
+            WatchedFile monitor = new WatchedFile(file);
+            watchedFiles.add(monitor);
+        }
+    }
+
+    /**
+     * Check file changed status, if so, send the notification.
+     */
+    private void checkAndNotify() {
+        if (System.currentTimeMillis() - lastCheckTimestamp < watchingPeriodInSec * 1000) {
+            // Don't reach the period threshold, ignore this check.
+            return;
+        }
 
 Review comment:
   Why do we need to double-check? To avoid the new added Monitor will re-check at once?
   IMO, we can set the check period as `watchingPeriodInSec`, because this case is a minority case and only affects the first check.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] codecov-io commented on issue #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

codecov-io commented on issue #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#issuecomment-596927662
 
 
   # [Codecov](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=h1) Report
   > Merging [#4475](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/ab8d7fab23b69593d7d06bdee20421006cb20189?src=pr&el=desc) will **increase** coverage by `0.06%`.
   > The diff coverage is `59.18%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/skywalking/pull/4475/graphs/tree.svg?width=650&token=qrILxY5yA8&height=150&src=pr)](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #4475      +/-   ##
   ==========================================
   + Coverage   24.97%   25.04%   +0.06%     
   ==========================================
     Files        1231     1232       +1     
     Lines       28496    28538      +42     
     Branches     3905     3913       +8     
   ==========================================
   + Hits         7118     7147      +29     
   - Misses      20719    20726       +7     
   - Partials      659      665       +6
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ap/server/core/storage/ttl/DataTTLKeeperTimer.java](https://codecov.io/gh/apache/skywalking/pull/4475/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvc3RvcmFnZS90dGwvRGF0YVRUTEtlZXBlclRpbWVyLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
   | [...ing/oap/server/library/util/FileChangeMonitor.java](https://codecov.io/gh/apache/skywalking/pull/4475/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItbGlicmFyeS9saWJyYXJ5LXV0aWwvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9saWJyYXJ5L3V0aWwvRmlsZUNoYW5nZU1vbml0b3IuamF2YQ==) | `67.44% <67.44%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=footer). Last update [ab8d7fa...f3fe485](https://codecov.io/gh/apache/skywalking/pull/4475?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on issue #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on issue #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#issuecomment-596919319
 
 
   @hanahmily I add the default implementation by following your prefer, please recheck.
   
   @kezhenxu94 Binding implementation added.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390071533
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   I am not sure whether there is that case, but I prefer to provide an input stream rather than byte[]. 
   Let's say in this way, if this class compare the changes, then we need extra memory space for the file contents(whole binary), but if we put the content into implementation, there is no extra memory cause, as you already have username/password configuration in provider config.
   
   I think your idea is only coming from the cert file, as you don't need to read it, right? For ES case, this is a double read or extra memory case.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390078616
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   What is `FileSystems.getDefault().newWatchService()` for? That doesn't look like a watcher with callback to me. Do I miss anything?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390081972
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   Another concern is noticing this
   ```
        * @throws  UnsupportedOperationException
        *          If this {@code FileSystem} does not support watching file system
        *          objects for changes and events. This exception is not thrown
        *          by {@code FileSystems} created by the default provider.
   ```
   Seems like we need to count on the JRE implementation somehow. I don't know the black magic inside now :)

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] hanahmily commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

hanahmily commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390078040
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   All right. Finally, we reach an agreement to implement `ContentChangedNotifier` to provide this feature. Thanks a lot.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390122221
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,188 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
+ * fetch the new content of the file and check with the prev one. If content changed, it will notify the listener.
+ *
+ * File Change
+ */
+@RequiredArgsConstructor
+@Slf4j
+public class FileChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    /**
+     * The list contains all monitors.
+     */
+    private static List<FileChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The absolute path of the monitored file.
+     */
+    private final String filePath;
+    /**
+     * Trigger notification when file is not there.
+     */
+    private final boolean acceptFileNotExisting;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    /**
+     * The notifier when file content changed.
+     */
+    private final FileChangedNotifier notifier;
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The last modify time of the {@link #filePath}
+     */
+    private long lastModifiedTimestamp = 0;
+
+    /**
+     * Start the file monitor for this instance.
+     */
+    public synchronized void start() {
 
 Review comment:
   the `synchronized` makes no sense, it's locked by the caller, and every caller is different from others

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390092032
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   My question is, when the exception happens, I never used this API before. Evaluating the real case.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390122265
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,188 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
+ * fetch the new content of the file and check with the prev one. If content changed, it will notify the listener.
+ *
+ * File Change
+ */
+@RequiredArgsConstructor
+@Slf4j
+public class FileChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    /**
+     * The list contains all monitors.
+     */
+    private static List<FileChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The absolute path of the monitored file.
+     */
+    private final String filePath;
+    /**
+     * Trigger notification when file is not there.
+     */
+    private final boolean acceptFileNotExisting;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    /**
+     * The notifier when file content changed.
+     */
+    private final FileChangedNotifier notifier;
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The last modify time of the {@link #filePath}
+     */
+    private long lastModifiedTimestamp = 0;
+
+    /**
+     * Start the file monitor for this instance.
+     */
+    public synchronized void start() {
+        if (FILE_MONITOR_TASK_SCHEDULER == null) {
+            FILE_MONITOR_TASK_SCHEDULER = Executors.newSingleThreadScheduledExecutor()
+                                                   .scheduleAtFixedRate(
+                                                       FileChangeMonitor::run, 1, 1,
+                                                       TimeUnit.SECONDS
+                                                   );
+        }
+
+        this.checkAndNotify();
+        MONITOR_INSTANCES.add(this);
+    }
+
+    public synchronized void stop() {
 
 Review comment:
   same here

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390123157
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,188 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
+ * fetch the new content of the file and check with the prev one. If content changed, it will notify the listener.
+ *
+ * File Change
+ */
+@RequiredArgsConstructor
+@Slf4j
+public class FileChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    /**
+     * The list contains all monitors.
+     */
+    private static List<FileChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The absolute path of the monitored file.
+     */
+    private final String filePath;
+    /**
+     * Trigger notification when file is not there.
+     */
+    private final boolean acceptFileNotExisting;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    /**
+     * The notifier when file content changed.
+     */
+    private final FileChangedNotifier notifier;
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The last modify time of the {@link #filePath}
+     */
+    private long lastModifiedTimestamp = 0;
+
+    /**
+     * Start the file monitor for this instance.
+     */
+    public synchronized void start() {
 
 Review comment:
   And you may want to prevent the monitor being started more than once

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390062605
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   > We should consider another scenario if the content of file doesn't change the monitor should not notify the related component.
   
   I expect this is the responsibility of notifier implementation. I just don't want to read the file twice.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390075169
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   better place it else where, in case the RAT plugin scans it occasionally

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390093242
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   This exception is rarely to see in the JDK classes. They only say this when necessary. I don't what this simple feature crashes.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] dmsolr commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

dmsolr commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390136989
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   I agree with @kezhenxu94 . JDK builtin WatchSerivce is a better choice for the file modified notify service.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390073899
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   Anyway, I could provide a default `ContentChangedNotifier` implementation, I think that will fit your wants?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390682966
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java
 ##########
 @@ -0,0 +1,242 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * MultipleFilesChangeMonitor provides the capability to detect file or multiple files changed. It provide second level
+ * change detection and feedback mechanism.
+ *
+ * Due to memory cost, this monitor mechanism is not suitable for small files and usually being changed on the runtime
+ * by user manually or 3rd party tool. Typical, these files are config information or authentication files.
+ */
+@Slf4j
+public class MultipleFilesChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    private static ReentrantLock SCHEDULER_CHANGE_LOCK = new ReentrantLock();
+    /**
+     * The list contains all monitors.
+     */
+    private static List<MultipleFilesChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    private List<WatchedFile> watchedFiles;
+    private FilesChangedNotifier notifier;
+
+    /**
+     * Create a new monitor for the given files
+     *
+     * @param watchingPeriodInSec The check period.
+     * @param notifier            to accept the file changed notification.
+     * @param files               to be monitored.
+     */
+    public MultipleFilesChangeMonitor(long watchingPeriodInSec,
+                                      FilesChangedNotifier notifier,
+                                      String... files) {
+        watchedFiles = new ArrayList<>();
+        this.watchingPeriodInSec = watchingPeriodInSec;
+        this.notifier = notifier;
+        for (final String file : files) {
+            WatchedFile monitor = new WatchedFile(file);
+            watchedFiles.add(monitor);
+        }
+    }
+
+    /**
+     * Check file changed status, if so, send the notification.
+     */
+    private void checkAndNotify() {
+        if (System.currentTimeMillis() - lastCheckTimestamp < watchingPeriodInSec * 1000) {
+            // Don't reach the period threshold, ignore this check.
+            return;
+        }
 
 Review comment:
   Because the case is, the timer period is certain and shorter than watcher period. We need to make sure the check period follows the user requirements.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] hanahmily commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

hanahmily commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390078040
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   All right. Finally, we reach an agreement to provide to implement `ContentChangedNotifier` to provide this feature. Thanks a lot.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390075349
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,156 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
 
 Review comment:
   > No. For ES and cert file, double read or extra memory is necessary.
   
   My point is for ES, we don't need extra memory, the user name and password are there, `ContentChangedNotifier` implementation doesn't have to hold it again(one in the ES storage config, the other in the watcher). I know your point to be easier for the watcher, but, this has a cost for ES implementation.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

kezhenxu94 commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390117131
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,188 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
+ * fetch the new content of the file and check with the prev one. If content changed, it will notify the listener.
+ *
+ * File Change
+ */
+@RequiredArgsConstructor
+@Slf4j
+public class FileChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    /**
+     * The list contains all monitors.
+     */
+    private static List<FileChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The absolute path of the monitored file.
+     */
+    private final String filePath;
+    /**
+     * Trigger notification when file is not there.
+     */
+    private final boolean acceptFileNotExisting;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    /**
+     * The notifier when file content changed.
+     */
+    private final FileChangedNotifier notifier;
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The last modify time of the {@link #filePath}
+     */
+    private long lastModifiedTimestamp = 0;
+
+    /**
+     * Start the file monitor for this instance.
+     */
+    public synchronized void start() {
+        if (FILE_MONITOR_TASK_SCHEDULER == null) {
+            FILE_MONITOR_TASK_SCHEDULER = Executors.newSingleThreadScheduledExecutor()
+                                                   .scheduleAtFixedRate(
+                                                       FileChangeMonitor::run, 1, 1,
+                                                       TimeUnit.SECONDS
+                                                   );
+        }
+
+        this.checkAndNotify();
+        MONITOR_INSTANCES.add(this);
+    }
+
+    public synchronized void stop() {
+        MONITOR_INSTANCES.remove(this);
+    }
+
+    /**
+     * Check the file status, if changed, send the notification.
+     */
+    private void checkAndNotify() {
+        if (System.currentTimeMillis() - lastCheckTimestamp < watchingPeriodInSec * 1000) {
+            // Don't reach the period threshold, ignore this check.
+            return;
+        }
+        File targetFile = new File(filePath);
+        if (!targetFile.exists() && acceptFileNotExisting) {
+            notifier.fileNotFound();
+        }
+        if (targetFile.isFile()) {
+            long lastModified = targetFile.lastModified();
+
+            if (lastModified != lastModifiedTimestamp) {
+                try (FileInputStream fileInputStream = new FileInputStream(targetFile)) {
+                    notifier.fileChanged(fileInputStream);
+                } catch (FileNotFoundException e) {
+                    log.error("The existed file turns to missing, watch file=" + filePath, e);
+                } catch (IOException e) {
+                    log.error("Read file failure, watch file=" + filePath, e);
+                } finally {
+                    lastModifiedTimestamp = lastModified;
+                }
+            }
+        }
+    }
+
+    /**
+     * Check all registered file.
+     */
+    private static void run() {
+        MONITOR_INSTANCES.forEach(monitor -> {
+            try {
+                monitor.checkAndNotify();
+            } catch (Throwable e) {
+                log.error("Error happens during monitoring file=" + monitor.filePath, e);
+            }
+        });
+    }
+
+    /**
+     * The callback when file changed.
+     */
+    public interface FileChangedNotifier {
+        /**
+         * Notify the new content by providing the file input stream
+         *
+         * @param readableStream points to the new content
+         * @throws IOException if error happens during reading.
+         */
+        void fileChanged(InputStream readableStream) throws IOException;
 
 Review comment:
   I don't think it's a good design, unless you're perfectly sure that the caller will always use the file content as an `InputStream`, otherwise, you create the USELESS input stream too eagerly

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390080937
 
 

 ##########
 File path: oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitorTest.java
 ##########
 @@ -0,0 +1,85 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.Charset;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class FileChangeMonitorTest {
+    private static String FILE_NAME = "FileChangeMonitorTest.tmp";
 
 Review comment:
   I think I clear the file in the end of test case. Where should I put? It is different in different OS systems, with different access control maybe.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on issue #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on issue #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#issuecomment-597092625
 
 
   @hanahmily @kezhenxu94 I submit the new implementation. As in CA cert scenario, there are multiple files which should be treated as an entity, I put these together and provide file content changes directly without `inputstream`/`findNotFound` notifications.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [skywalking] wu-sheng commented on a change in pull request #4475: Add file change detection mechanism

Posted by GitBox <gi...@apache.org>.

wu-sheng commented on a change in pull request #4475: Add file change detection mechanism
URL: https://github.com/apache/skywalking/pull/4475#discussion_r390123620
 
 

 ##########
 File path: oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/FileChangeMonitor.java
 ##########
 @@ -0,0 +1,188 @@
+/*
+ * 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.skywalking.oap.server.library.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * File change monitor is a disk file watcher. It keeps to watch the file `last modified timestamp`, after it changed,
+ * fetch the new content of the file and check with the prev one. If content changed, it will notify the listener.
+ *
+ * File Change
+ */
+@RequiredArgsConstructor
+@Slf4j
+public class FileChangeMonitor {
+    /**
+     * The backend scheduler to trigger all file monitoring.
+     */
+    private static ScheduledFuture<?> FILE_MONITOR_TASK_SCHEDULER;
+    /**
+     * The list contains all monitors.
+     */
+    private static List<FileChangeMonitor> MONITOR_INSTANCES = new ArrayList<>();
+
+    /**
+     * The absolute path of the monitored file.
+     */
+    private final String filePath;
+    /**
+     * Trigger notification when file is not there.
+     */
+    private final boolean acceptFileNotExisting;
+    /**
+     * The period of watching thread checking the file status. Unit is the second.
+     */
+    private final long watchingPeriodInSec;
+    /**
+     * The notifier when file content changed.
+     */
+    private final FileChangedNotifier notifier;
+    /**
+     * The timestamp when last time do status checked.
+     */
+    private long lastCheckTimestamp = 0;
+    /**
+     * The last modify time of the {@link #filePath}
+     */
+    private long lastModifiedTimestamp = 0;
+
+    /**
+     * Start the file monitor for this instance.
+     */
+    public synchronized void start() {
+        if (FILE_MONITOR_TASK_SCHEDULER == null) {
+            FILE_MONITOR_TASK_SCHEDULER = Executors.newSingleThreadScheduledExecutor()
+                                                   .scheduleAtFixedRate(
+                                                       FileChangeMonitor::run, 1, 1,
+                                                       TimeUnit.SECONDS
+                                                   );
+        }
+
+        this.checkAndNotify();
+        MONITOR_INSTANCES.add(this);
+    }
+
+    public synchronized void stop() {
+        MONITOR_INSTANCES.remove(this);
+    }
+
+    /**
+     * Check the file status, if changed, send the notification.
+     */
+    private void checkAndNotify() {
+        if (System.currentTimeMillis() - lastCheckTimestamp < watchingPeriodInSec * 1000) {
+            // Don't reach the period threshold, ignore this check.
+            return;
+        }
+        File targetFile = new File(filePath);
+        if (!targetFile.exists() && acceptFileNotExisting) {
+            notifier.fileNotFound();
+        }
+        if (targetFile.isFile()) {
+            long lastModified = targetFile.lastModified();
+
+            if (lastModified != lastModifiedTimestamp) {
+                try (FileInputStream fileInputStream = new FileInputStream(targetFile)) {
+                    notifier.fileChanged(fileInputStream);
+                } catch (FileNotFoundException e) {
+                    log.error("The existed file turns to missing, watch file=" + filePath, e);
+                } catch (IOException e) {
+                    log.error("Read file failure, watch file=" + filePath, e);
+                } finally {
+                    lastModifiedTimestamp = lastModified;
+                }
+            }
+        }
+    }
+
+    /**
+     * Check all registered file.
+     */
+    private static void run() {
+        MONITOR_INSTANCES.forEach(monitor -> {
+            try {
+                monitor.checkAndNotify();
+            } catch (Throwable e) {
+                log.error("Error happens during monitoring file=" + monitor.filePath, e);
+            }
+        });
+    }
+
+    /**
+     * The callback when file changed.
+     */
+    public interface FileChangedNotifier {
+        /**
+         * Notify the new content by providing the file input stream
+         *
+         * @param readableStream points to the new content
+         * @throws IOException if error happens during reading.
+         */
+        void fileChanged(InputStream readableStream) throws IOException;
 
 Review comment:
   What else could they use?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services