You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "gautamguptabasant (via GitHub)" <gi...@apache.org> on 2023/09/21 17:28:15 UTC

[GitHub] [gobblin] gautamguptabasant opened a new pull request, #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

gautamguptabasant opened a new pull request, #3786:
URL: https://github.com/apache/gobblin/pull/3786

   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
   - [ ] 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-1917
   
   
   ### Description
   - Updating the log messages in the Salesforce related classes.
   - In some cases, the exception was not included in the log. Adding it to make debugging issues easier.
   - For some cases, added more details in the logs to print the current progress of the tasks.
   - Minor coding style updates as well to improve code readability.
   - There are no functional changes in this PR
   
   ### Tests
   - No changes around tests, as no functional changes as part of this PR
   
   
   ### Commits
   - [ ] 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.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3786:
URL: https://github.com/apache/gobblin/pull/3786#discussion_r1333814434


##########
gobblin-salesforce/src/main/java/org/apache/gobblin/salesforce/SalesforceExtractor.java:
##########
@@ -504,10 +479,7 @@ private static String buildUrl(String path, List<NameValuePair> qparams) throws
   }
 
   private static boolean isNullPredicate(List<Predicate> predicateList) {
-    if (predicateList == null || predicateList.size() == 0) {
-      return true;
-    }
-    return false;

Review Comment:
   I can't believe this was left in here, nice catch! 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] Will-Lo merged pull request #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo merged PR #3786:
URL: https://github.com/apache/gobblin/pull/3786


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] gautamguptabasant commented on a diff in pull request #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

Posted by "gautamguptabasant (via GitHub)" <gi...@apache.org>.
gautamguptabasant commented on code in PR #3786:
URL: https://github.com/apache/gobblin/pull/3786#discussion_r1333402625


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixJobTask.java:
##########
@@ -211,14 +211,15 @@ public TaskResult run() {
       try {
         this.jobsMapping.deleteMapping(jobUri);
       } catch (Exception e) {
+        log.warn("Failed to delete jobs mapping for job: {}", jobUri, e);
         return new TaskResult(TaskResult.Status.FAILED,"Cannot delete jobs mapping for job : " + jobUri);
       }
     }
   }
 
   @Override
   public void cancel() {
-    log.info("Cancelling planning job {}", this.planningJobId);
+    log.info("Cancelling planning job {} | jobTaskMetrics: {}", this.planningJobId, this.jobTaskMetrics);

Review Comment:
   Thanks for catching this, fixed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] gautamguptabasant commented on a diff in pull request #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

Posted by "gautamguptabasant (via GitHub)" <gi...@apache.org>.
gautamguptabasant commented on code in PR #3786:
URL: https://github.com/apache/gobblin/pull/3786#discussion_r1333811858


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixJobTask.java:
##########
@@ -219,7 +219,7 @@ public TaskResult run() {
 
   @Override
   public void cancel() {
-    log.info("Cancelling planning job {} | jobTaskMetrics: {}", this.planningJobId, this.jobTaskMetrics);
+    log.info("Cancelling planning job {} | jobTaskMetrics: {}", this.planningJobId, this.jobTaskMetrics.getMetrics());

Review Comment:
   Hmm, somehow I read this map as <String, String>. Yes, this does look a bit tricky to log. Omitting this change for now to get the rest of it in. Will revisit this one 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.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3786:
URL: https://github.com/apache/gobblin/pull/3786#discussion_r1333741471


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixJobTask.java:
##########
@@ -219,7 +219,7 @@ public TaskResult run() {
 
   @Override
   public void cancel() {
-    log.info("Cancelling planning job {} | jobTaskMetrics: {}", this.planningJobId, this.jobTaskMetrics);
+    log.info("Cancelling planning job {} | jobTaskMetrics: {}", this.planningJobId, this.jobTaskMetrics.getMetrics());

Review Comment:
   I think this still has an issue:
   Going into the implementation of `StandardMetrics.getMetrics()` it returns a `Map<String, Metric>`. When this is printed it would still return the reference to the map instead of the contents. If you really want to print the metrics here, we will need a custom log function where you traverse the Map<String, Metric> and then print out the contexts as appropriate. But given that `Metric` is just an interface in `metrics-core-4.1.2`, I think this would be tricky as we would have to also implement a switch statement checking each type...
   
   Which metric are you interested in here? Would it be possible just to limit the scope to print only those metrics?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] codecov-commenter commented on pull request #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3786:
URL: https://github.com/apache/gobblin/pull/3786#issuecomment-1730032641

   ## [Codecov](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#3786](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (244abab) into [master](https://app.codecov.io/gh/apache/gobblin/commit/fc73db8b3feb0fb70a6bfde983646eecaf826ef4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fc73db8) will **decrease** coverage by `1.26%`.
   > The diff coverage is `15.38%`.
   
   > :exclamation: Current head 244abab differs from pull request most recent head 5e139db. Consider uploading reports for the commit 5e139db to get more accurate results
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3786      +/-   ##
   ============================================
   - Coverage     47.29%   46.03%   -1.26%     
   + Complexity    10948     2178    -8770     
   ============================================
     Files          2152      416    -1736     
     Lines         85104    17965   -67139     
     Branches       9452     2191    -7261     
   ============================================
   - Hits          40247     8271   -31976     
   + Misses        41201     8810   -32391     
   + Partials       3656      884    -2772     
   ```
   
   
   | [Files Changed](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...rg/apache/gobblin/cluster/GobblinHelixJobTask.java](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4Sm9iVGFzay5qYXZh) | `59.57% <0.00%> (-0.65%)` | :arrow_down: |
   | [.../apache/gobblin/salesforce/BulkResultIterator.java](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvQnVsa1Jlc3VsdEl0ZXJhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...gobblin/salesforce/SalesforceHistogramService.java](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvU2FsZXNmb3JjZUhpc3RvZ3JhbVNlcnZpY2UuamF2YQ==) | `4.19% <0.00%> (ø)` | |
   | [...apache/gobblin/salesforce/SalesforceExtractor.java](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvU2FsZXNmb3JjZUV4dHJhY3Rvci5qYXZh) | `14.61% <18.42%> (-0.16%)` | :arrow_down: |
   | [...rg/apache/gobblin/salesforce/SalesforceSource.java](https://app.codecov.io/gh/apache/gobblin/pull/3786?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z29iYmxpbi1zYWxlc2ZvcmNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NhbGVzZm9yY2UvU2FsZXNmb3JjZVNvdXJjZS5qYXZh) | `46.41% <100.00%> (-0.26%)` | :arrow_down: |
   
   ... and [1741 files with indirect coverage changes](https://app.codecov.io/gh/apache/gobblin/pull/3786/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3786: [GOBBLIN-1917] Logging updates for Salesforce classes

Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3786:
URL: https://github.com/apache/gobblin/pull/3786#discussion_r1333398355


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixJobTask.java:
##########
@@ -211,14 +211,15 @@ public TaskResult run() {
       try {
         this.jobsMapping.deleteMapping(jobUri);
       } catch (Exception e) {
+        log.warn("Failed to delete jobs mapping for job: {}", jobUri, e);
         return new TaskResult(TaskResult.Status.FAILED,"Cannot delete jobs mapping for job : " + jobUri);
       }
     }
   }
 
   @Override
   public void cancel() {
-    log.info("Cancelling planning job {}", this.planningJobId);
+    log.info("Cancelling planning job {} | jobTaskMetrics: {}", this.planningJobId, this.jobTaskMetrics);

Review Comment:
   If you're logging the metrics like this it would just print a reference to the java object? Is that intended?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org