You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/03/13 01:32:58 UTC

[GitHub] [incubator-gobblin] jack-moseley opened a new pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

jack-moseley opened a new pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924
 
 
   Dear Gobblin maintainers,
   
   Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
   
   
   ### JIRA
   - [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1084
   
   
   ### Description
   - [x] Here are some details about my PR, including screenshots (if applicable):
   
   Currently, if edges fail to be added on startup, they will not be tried again even if the issue is fixed through modifying the templates. This change causes the entire flowgraph to be refreshed if any template is modified.
   
   ### Tests
   - [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   
   
   ### Commits
   - [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
       1. Subject is separated from body by a blank line
       2. Subject is limited to 50 characters
       3. Subject does not end with a period
       4. Subject uses the imperative mood ("add", not "adding")
       5. Body wraps at 72 characters
       6. Body explains "what" and "why", not "how"
   
   

----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392551050
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/template_catalog/ObservingFSFlowEdgeTemplateCatalog.java
 ##########
 @@ -49,6 +51,10 @@
   private Map<URI, List<JobTemplate>> jobTemplateMap = new ConcurrentHashMap<>();
   private ReadWriteLock rwLock;
 
+  @Getter
+  @Setter
+  private boolean shouldRefreshFlowGraph = false;
 
 Review comment:
   Sorry, I did not mean to add a lock at this assignment. But my comment was more to ensure that access to the variable is guarded by a lock. 

----------------------------------------------------------------
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] [incubator-gobblin] asfgit closed pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924
 
 
   

----------------------------------------------------------------
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] [incubator-gobblin] codecov-io edited a comment on issue #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#issuecomment-600366892
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=h1) Report
   > Merging [#2924](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/5796bb4c890cf4c7e48c061907232289cfb08ab9&el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `57.14%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2924      +/-   ##
   ============================================
   - Coverage     45.46%   45.45%   -0.02%     
   - Complexity     9109     9113       +4     
   ============================================
     Files          1936     1936              
     Lines         73059    73145      +86     
     Branches       8051     8071      +20     
   ============================================
   + Hits          33217    33246      +29     
   - Misses        36782    36828      +46     
   - Partials       3060     3071      +11     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...blin/service/modules/core/GitFlowGraphMonitor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dpdEZsb3dHcmFwaE1vbml0b3IuamF2YQ==) | `72.00% <0.00%> (-1.78%)` | `23.00 <1.00> (ø)` | |
   | [...lin/service/modules/core/GitMonitoringService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dpdE1vbml0b3JpbmdTZXJ2aWNlLmphdmE=) | `57.59% <ø> (ø)` | `9.00 <0.00> (ø)` | |
   | [...odules/template\_catalog/FSFlowTemplateCatalog.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy90ZW1wbGF0ZV9jYXRhbG9nL0ZTRmxvd1RlbXBsYXRlQ2F0YWxvZy5qYXZh) | `59.57% <100.00%> (+0.87%)` | `9.00 <1.00> (+1.00)` | |
   | [...te\_catalog/ObservingFSFlowEdgeTemplateCatalog.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy90ZW1wbGF0ZV9jYXRhbG9nL09ic2VydmluZ0ZTRmxvd0VkZ2VUZW1wbGF0ZUNhdGFsb2cuamF2YQ==) | `88.23% <100.00%> (+1.13%)` | `9.00 <1.00> (+1.00)` | |
   | [...obblin/service/monitoring/FlowStatusGenerator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9GbG93U3RhdHVzR2VuZXJhdG9yLmphdmE=) | `46.00% <0.00%> (-23.70%)` | `11.00% <0.00%> (ø%)` | |
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `71.33% <0.00%> (-5.14%)` | `13.00% <0.00%> (ø%)` | |
   | [...e/gobblin/runtime/locks/ZookeeperBasedJobLock.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbG9ja3MvWm9va2VlcGVyQmFzZWRKb2JMb2NrLmphdmE=) | `63.33% <0.00%> (-1.12%)` | `15.00% <0.00%> (-1.00%)` | |
   | [...he/gobblin/compaction/source/CompactionSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25Tb3VyY2UuamF2YQ==) | `72.16% <0.00%> (-0.90%)` | `14.00% <0.00%> (ø%)` | |
   | [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `30.20% <0.00%> (-0.68%)` | `24.00% <0.00%> (-1.00%)` | |
   | [...service/modules/orchestration/DagManagerUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXJVdGlscy5qYXZh) | `85.05% <0.00%> (-0.49%)` | `39.00% <0.00%> (+2.00%)` | :arrow_down: |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?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/incubator-gobblin/pull/2924?src=pr&el=footer). Last update [5796bb4...eafe0db](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392484183
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitMonitoringService.java
 ##########
 @@ -304,7 +304,7 @@ protected void shutDown() throws Exception {
      * @throws GitAPIException
      * @throws IOException
      */
-    private void initRepository() throws GitAPIException, IOException {
+    public void initRepository() throws GitAPIException, IOException {
 
 Review comment:
   package private instead of public?

----------------------------------------------------------------
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] [incubator-gobblin] jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392524602
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -127,6 +128,14 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
+    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
 
 Review comment:
   I didn't do this since refresh flow graph only makes sense for observing flow catalog, but yeah I guess doing instanceof is not great either so I'll change 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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392484539
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/template_catalog/ObservingFSFlowEdgeTemplateCatalog.java
 ##########
 @@ -49,6 +51,10 @@
   private Map<URI, List<JobTemplate>> jobTemplateMap = new ConcurrentHashMap<>();
   private ReadWriteLock rwLock;
 
+  @Getter
+  @Setter
+  private boolean shouldRefreshFlowGraph = false;
 
 Review comment:
   See comment earlier about making access of this variable thread safe. 

----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392551209
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -128,11 +131,15 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
-    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
-      ObservingFSFlowEdgeTemplateCatalog catalog = (ObservingFSFlowEdgeTemplateCatalog) flowTemplateCatalog.get();
-      if (catalog.isShouldRefreshFlowGraph()) {
-        this.gitRepo.initRepository();
-        catalog.setShouldRefreshFlowGraph(false);
+    if (lock.tryLock()) {
 
 Review comment:
   Shouldn't we be using the lock defined in the ObservingFlowCatalog here? Defining a new lock here is not useful since the GitFlowGraphMonitor is single threaded. 

----------------------------------------------------------------
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] [incubator-gobblin] jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r393250210
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -128,11 +131,15 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
-    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
-      ObservingFSFlowEdgeTemplateCatalog catalog = (ObservingFSFlowEdgeTemplateCatalog) flowTemplateCatalog.get();
-      if (catalog.isShouldRefreshFlowGraph()) {
-        this.gitRepo.initRepository();
-        catalog.setShouldRefreshFlowGraph(false);
+    if (lock.tryLock()) {
 
 Review comment:
   Makes sense, updated to use the same lock as the catalog.

----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r393159757
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -128,11 +131,15 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
-    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
-      ObservingFSFlowEdgeTemplateCatalog catalog = (ObservingFSFlowEdgeTemplateCatalog) flowTemplateCatalog.get();
-      if (catalog.isShouldRefreshFlowGraph()) {
-        this.gitRepo.initRepository();
-        catalog.setShouldRefreshFlowGraph(false);
+    if (lock.tryLock()) {
 
 Review comment:
   So here is the race condition I was thinking of: 
   1. FlowCatalog has been refreshed so shouldRefreshFlowGraph is marked true. 
   2. GitMonitoringService calls processGitConfigChanges() and at some point before this method is done, FlowCatalog observes another refresh and sets shouldRefreshFlowGraph flag to true. 
   3. This flag is reset by GitMonitoringService to false at the end of processGitConfigChanges(). 
   
   So the GitMonitoringService can miss the 2nd catalog refresh and fail to add edges. LMK, if this is still not clear.

----------------------------------------------------------------
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] [incubator-gobblin] jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392525349
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/template_catalog/ObservingFSFlowEdgeTemplateCatalog.java
 ##########
 @@ -49,6 +51,10 @@
   private Map<URI, List<JobTemplate>> jobTemplateMap = new ConcurrentHashMap<>();
   private ReadWriteLock rwLock;
 
+  @Getter
+  @Setter
+  private boolean shouldRefreshFlowGraph = false;
 
 Review comment:
   Is it necessary to add another lock here? I've added a lock in the flowgraphmonitor (which is where it gets read/set to false), and there is already a lock in this class on when it's set to true.

----------------------------------------------------------------
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] [incubator-gobblin] codecov-io commented on issue #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#issuecomment-600366892
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=h1) Report
   > Merging [#2924](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/5796bb4c890cf4c7e48c061907232289cfb08ab9&el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `57.14%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2924      +/-   ##
   ============================================
   - Coverage     45.46%   45.45%   -0.02%     
   - Complexity     9109     9113       +4     
   ============================================
     Files          1936     1936              
     Lines         73059    73145      +86     
     Branches       8051     8071      +20     
   ============================================
   + Hits          33217    33246      +29     
   - Misses        36782    36828      +46     
   - Partials       3060     3071      +11     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...blin/service/modules/core/GitFlowGraphMonitor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dpdEZsb3dHcmFwaE1vbml0b3IuamF2YQ==) | `72.00% <0.00%> (-1.78%)` | `23.00 <1.00> (ø)` | |
   | [...lin/service/modules/core/GitMonitoringService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dpdE1vbml0b3JpbmdTZXJ2aWNlLmphdmE=) | `57.59% <ø> (ø)` | `9.00 <0.00> (ø)` | |
   | [...odules/template\_catalog/FSFlowTemplateCatalog.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy90ZW1wbGF0ZV9jYXRhbG9nL0ZTRmxvd1RlbXBsYXRlQ2F0YWxvZy5qYXZh) | `59.57% <100.00%> (+0.87%)` | `9.00 <1.00> (+1.00)` | |
   | [...te\_catalog/ObservingFSFlowEdgeTemplateCatalog.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy90ZW1wbGF0ZV9jYXRhbG9nL09ic2VydmluZ0ZTRmxvd0VkZ2VUZW1wbGF0ZUNhdGFsb2cuamF2YQ==) | `88.23% <100.00%> (+1.13%)` | `9.00 <1.00> (+1.00)` | |
   | [...obblin/service/monitoring/FlowStatusGenerator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9GbG93U3RhdHVzR2VuZXJhdG9yLmphdmE=) | `46.00% <0.00%> (-23.70%)` | `11.00% <0.00%> (ø%)` | |
   | [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `71.33% <0.00%> (-5.14%)` | `13.00% <0.00%> (ø%)` | |
   | [...e/gobblin/runtime/locks/ZookeeperBasedJobLock.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvbG9ja3MvWm9va2VlcGVyQmFzZWRKb2JMb2NrLmphdmE=) | `63.33% <0.00%> (-1.12%)` | `15.00% <0.00%> (-1.00%)` | |
   | [...he/gobblin/compaction/source/CompactionSource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb21wYWN0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbXBhY3Rpb24vc291cmNlL0NvbXBhY3Rpb25Tb3VyY2UuamF2YQ==) | `72.16% <0.00%> (-0.90%)` | `14.00% <0.00%> (ø%)` | |
   | [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `30.20% <0.00%> (-0.68%)` | `24.00% <0.00%> (-1.00%)` | |
   | [...service/modules/orchestration/DagManagerUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXJVdGlscy5qYXZh) | `85.05% <0.00%> (-0.49%)` | `39.00% <0.00%> (+2.00%)` | :arrow_down: |
   | ... and [2 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2924/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?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/incubator-gobblin/pull/2924?src=pr&el=footer). Last update [5796bb4...eafe0db](https://codecov.io/gh/apache/incubator-gobblin/pull/2924?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] [incubator-gobblin] jack-moseley commented on issue #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on issue #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#issuecomment-598897296
 
 
   @sv2000 please review

----------------------------------------------------------------
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] [incubator-gobblin] jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392850365
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -128,11 +131,15 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
-    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
-      ObservingFSFlowEdgeTemplateCatalog catalog = (ObservingFSFlowEdgeTemplateCatalog) flowTemplateCatalog.get();
-      if (catalog.isShouldRefreshFlowGraph()) {
-        this.gitRepo.initRepository();
-        catalog.setShouldRefreshFlowGraph(false);
+    if (lock.tryLock()) {
 
 Review comment:
   Hmm okay that's what I was thinking but I'm a bit confused what the concurrency issue is then. I guess the problem is that this variable could be modified from both git monitor and catalog at the same time?
   
   In that case it should work to modify the `setShouldRefreshFlowGraph` in catalog to acquire the same rwlock right?

----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392483983
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -127,6 +128,14 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
+    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
 
 Review comment:
   We shouldn't be doing this check on every processGitCOnfigChanges() call.  Can't we just have a method isShouldRefreshFlowGraph() in the parent Catalog class that returns false and have this method overriden in the observing catalog child class?

----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392481458
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -127,6 +128,14 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
+    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
+      ObservingFSFlowEdgeTemplateCatalog catalog = (ObservingFSFlowEdgeTemplateCatalog) flowTemplateCatalog.get();
+      if (catalog.isShouldRefreshFlowGraph()) {
+        this.gitRepo.initRepository();
 
 Review comment:
   Add a log line here indicating that we have detected updates in the template catalog and that we need to re-bootstrap the FlowGraph.

----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r393159757
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -128,11 +131,15 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
-    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
-      ObservingFSFlowEdgeTemplateCatalog catalog = (ObservingFSFlowEdgeTemplateCatalog) flowTemplateCatalog.get();
-      if (catalog.isShouldRefreshFlowGraph()) {
-        this.gitRepo.initRepository();
-        catalog.setShouldRefreshFlowGraph(false);
+    if (lock.tryLock()) {
 
 Review comment:
   So here is the race condition I was thinking of: 
   1. FlowCatalog has been refreshed, so it sets shouldRefreshFlowGraph to true. 
   2. GitMonitoringService then calls processGitConfigChanges(), and at some point before the end of this method, FlowCatalog observes another refresh and sets shouldRefreshFlowGraph flag to true. 
   3. shouldRefreshFlowGraph is reset by GitMonitoringService to false at the end of processGitConfigChanges(). 
   
   So the GitMonitoringService can miss the 2nd catalog refresh and fail to add edges. LMK, if this is still not clear.

----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified

Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #2924: [GOBBLIN-1084] Refresh flowgraph when templates are modified
URL: https://github.com/apache/incubator-gobblin/pull/2924#discussion_r392481084
 
 

 ##########
 File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
 ##########
 @@ -127,6 +128,14 @@ public boolean shouldPollGit() {
    */
   @Override
   void processGitConfigChanges() throws GitAPIException, IOException {
+    if (flowTemplateCatalog.isPresent() && (flowTemplateCatalog.get() instanceof ObservingFSFlowEdgeTemplateCatalog)) {
+      ObservingFSFlowEdgeTemplateCatalog catalog = (ObservingFSFlowEdgeTemplateCatalog) flowTemplateCatalog.get();
+      if (catalog.isShouldRefreshFlowGraph()) {
+        this.gitRepo.initRepository();
+        catalog.setShouldRefreshFlowGraph(false);
 
 Review comment:
   shouldn't resetting this boolean variable be inside a lock? 

----------------------------------------------------------------
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