[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335048567
 
 

 ##
 File path: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcher.java
 ##
 @@ -69,6 +69,37 @@
  */
 class HistoryServerArchiveFetcher {
 
+   /**
+* Possible job archive operations in history-server.
+*/
+   public enum JobArchiveOperation {
+   /** Job archive was found in one refresh location and created 
in history server. */
+   CREATED,
+   /** Job archive was deleted from one of refresh locations and 
deleted from history server.*/
+   DELETED
+   }
+
+   /**
+* Representation of job archive event.
+*/
+   public static class JobArchiveEvent {
 
 Review comment:
   We're only dealing with jobs in this component, so `ArchiveEvent` would be 
more succinct.


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


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335047611
 
 

 ##
 File path: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServer.java
 ##
 @@ -163,6 +175,8 @@ public HistoryServer(Configuration config, CountDownLatch 
numArchivedJobs) throw
}
webDir = new File(webDirectory);
 
+   boolean processArchiveDeletion = 
config.getBoolean(HistoryServerOptions.HISTORY_SERVER_PROCESS_JOB_DELETION);
 
 Review comment:
   I think `cleanExpiredJobs` or `removeExpiredJobs` better describes this 
variable.


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


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335048872
 
 

 ##
 File path: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcher.java
 ##
 @@ -69,6 +69,37 @@
  */
 class HistoryServerArchiveFetcher {
 
+   /**
+* Possible job archive operations in history-server.
+*/
+   public enum JobArchiveOperation {
 
 Review comment:
   ```suggestion
public enum ArchiveEventType {
   ```
   
   I think type better describes this, because the other class is called 
`(Job)ArchiveEvent`.


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


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335047870
 
 

 ##
 File path: 
flink-core/src/main/java/org/apache/flink/configuration/HistoryServerOptions.java
 ##
 @@ -89,6 +89,16 @@
.withDescription("Enable HTTPs access to the 
HistoryServer web frontend. This is applicable only when the" +
" global SSL flag security.ssl.enabled is set 
to true.");
 
+   /**
+* Specifies whether should HistoryServer process job archive deletion. 
If this option is enabled
+* then deleted job archives are also deleted from HistoryServer. By 
default is disabled.
+*/
+   public static final ConfigOption 
HISTORY_SERVER_PROCESS_JOB_DELETION =
 
 Review comment:
   ```suggestion
public static final ConfigOption 
HISTORY_SERVER_CLEANUP_EXPIRED_JOBS =
   ```
   
   or something more like 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:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335049403
 
 

 ##
 File path: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcher.java
 ##
 @@ -79,9 +110,15 @@
private final JobArchiveFetcherTask fetcherTask;
private final long refreshIntervalMillis;
 
-   HistoryServerArchiveFetcher(long refreshIntervalMillis, 
List refreshDirs, File webDir, CountDownLatch 
numArchivedJobs) {
+   HistoryServerArchiveFetcher(
+   long refreshIntervalMillis,
+   List refreshDirs,
+   File webDir,
+   Consumer jobArchiveEventListener,
+   boolean processArchiveDeletion
 
 Review comment:
   ```suggestion
boolean cleanupExpiredArchives
   ```
   
   or whatever the name of this is in the `HistoryServer`.


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


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335049733
 
 

 ##
 File path: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcher.java
 ##
 @@ -223,17 +267,40 @@ public void run() {
}
}
}
-   if (updateOverview) {
-   
updateJobOverview(webOverviewDir, webDir);
-   for (int x = 0; x < 
numFetchedArchives; x++) {
-   
numArchivedJobs.countDown();
-   }
-   }
}
+
+   if (!jobsToRemove.isEmpty() && 
processArchiveDeletion) {
+   
operations.addAll(removeDeletedJobs(jobsToRemove));
+   }
+   if (!operations.isEmpty()) {
+   updateJobOverview(webOverviewDir, 
webDir);
+   }
+   
operations.forEach(jobArchiveEventListener::accept);
} catch (Exception e) {
LOG.error("Critical failure while 
fetching/processing job archives.", e);
}
}
+
+   private List removeDeletedJobs(Set 
jobsToRemove) {
 
 Review comment:
   Maybe `cleanupExpiredJobs`?


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


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335049547
 
 

 ##
 File path: 
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServerArchiveFetcher.java
 ##
 @@ -137,6 +181,8 @@ void stop() {
@Override
public void run() {
try {
+   List operations = new 
ArrayList<>();
 
 Review comment:
   ```suggestion
List events = new 
ArrayList<>();
   ```


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


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335046719
 
 

 ##
 File path: 
flink-core/src/main/java/org/apache/flink/configuration/HistoryServerOptions.java
 ##
 @@ -89,6 +89,16 @@
.withDescription("Enable HTTPs access to the 
HistoryServer web frontend. This is applicable only when the" +
" global SSL flag security.ssl.enabled is set 
to true.");
 
+   /**
+* Specifies whether should HistoryServer process job archive deletion. 
If this option is enabled
+* then deleted job archives are also deleted from HistoryServer. By 
default is disabled.
 
 Review comment:
   nitpick:
   ```
   Specifies whether the HistoryServer should process job archive deletion. If 
this option is enabled then deleted job archives are also deleted from 
HistoryServer.
   ```


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


With regards,
Apache Git Services

[GitHub] [flink] aljoscha commented on a change in pull request #9759: [FLINK-14169][history-server] Cleanup expired jobs from history server

2019-10-15 Thread GitBox 
aljoscha commented on a change in pull request #9759: 
[FLINK-14169][history-server] Cleanup expired jobs from history server
URL: https://github.com/apache/flink/pull/9759#discussion_r335046093
 
 

 ##
 File path: docs/_includes/generated/history_server_configuration.html
 ##
 @@ -22,6 +22,11 @@
 (none)
 Address of the HistoryServer's web interface.
 
+
+historyserver.web.clean-expired-jobs
+false
+Whether HistoryServer should cleanup jobs, that are no longer 
present in historyserver.archive.fs.dir.
 
 Review comment:
   ```suggestion
   Whether HistoryServer should cleanup jobs that are no longer 
present in historyserver.archive.fs.dir.
   ```


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


With regards,
Apache Git Services