You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2024/01/03 09:08:49 UTC

[PR] Use higher fetch timeout for Kinesis [pinot]

KKcorps opened a new pull request, #12214:
URL: https://github.com/apache/pinot/pull/12214

   Currently, we run into a scenario where KinesisConsumer gets stuck due to timeouts.
   
   The scenario is explained in the attached diagram
   
   ![Getting Started_ Boards (1) (1)](https://github.com/apache/pinot/assets/5559703/470a74c8-bef2-40d8-8dc9-b3b88930bc67)
   
   
   The temporary solution is to increase the timeout of kinesis. A more permanent solution would be to modify the fetchRecords method to accept a `emitter` function that can be called from a forever running while loop to push the data.
   


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Use higher fetch timeout for Kinesis [pinot]

Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps merged PR #12214:
URL: https://github.com/apache/pinot/pull/12214


-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Use higher fetch timeout for Kinesis [pinot]

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

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12214?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: `2 lines` in your changes are missing coverage. Please review.
   > Comparison is base [(`d4d910c`)](https://app.codecov.io/gh/apache/pinot/commit/d4d910cec11569c5a0b6f986b361e0b3c1903962?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.51% compared to head [(`3c8fa78`)](https://app.codecov.io/gh/apache/pinot/pull/12214?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 27.69%.
   > Report is 1 commits behind head on master.
   
   | [Files](https://app.codecov.io/gh/apache/pinot/pull/12214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...java/org/apache/pinot/spi/stream/StreamConfig.java](https://app.codecov.io/gh/apache/pinot/pull/12214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvc3RyZWFtL1N0cmVhbUNvbmZpZy5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #12214       +/-   ##
   =============================================
   - Coverage     61.51%   27.69%   -33.83%     
   + Complexity      207      201        -6     
   =============================================
     Files          2415     2415               
     Lines        131167   131168        +1     
     Branches      20245    20246        +1     
   =============================================
   - Hits          80693    36332    -44361     
   - Misses        44573    92088    +47515     
   + Partials       5901     2748     -3153     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration1](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration2](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-11](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-33.78%)` | :arrow_down: |
   | [java-21](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-33.72%)` | :arrow_down: |
   | [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-33.81%)` | :arrow_down: |
   | [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-33.70%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-33.83%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-33.82%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12214?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Use higher fetch timeout for Kinesis [pinot]

Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #12214:
URL: https://github.com/apache/pinot/pull/12214#discussion_r1441045099


##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -142,7 +143,10 @@ public StreamConfig(String tableNameWithType, Map<String, String> streamConfigMa
     }
     _connectionTimeoutMillis = connectionTimeoutMillis;
 
-    int fetchTimeoutMillis = DEFAULT_STREAM_FETCH_TIMEOUT_MILLIS;
+    // For Kinesis, we need to set a higher fetch timeout to avoid getting stuck in empty records loop
+    int fetchTimeoutMillis =
+        !_consumerFactoryClassName.contains("KinesisConsumerFactory") ? DEFAULT_STREAM_FETCH_TIMEOUT_MILLIS_KINESIS

Review Comment:
   Let's at least add `TODO` comment to clean up this part not to expose Kinesis concept if possible.
   
   Some potential approach is to use 10min global default timeout if Kafka side will not be affected by bumping up the timeout
   



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Use higher fetch timeout for Kinesis [pinot]

Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #12214:
URL: https://github.com/apache/pinot/pull/12214#discussion_r1452023748


##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -142,7 +143,10 @@ public StreamConfig(String tableNameWithType, Map<String, String> streamConfigMa
     }
     _connectionTimeoutMillis = connectionTimeoutMillis;
 
-    int fetchTimeoutMillis = DEFAULT_STREAM_FETCH_TIMEOUT_MILLIS;
+    // For Kinesis, we need to set a higher fetch timeout to avoid getting stuck in empty records loop
+    int fetchTimeoutMillis =
+        !_consumerFactoryClassName.contains("KinesisConsumerFactory") ? DEFAULT_STREAM_FETCH_TIMEOUT_MILLIS_KINESIS

Review Comment:
   Yeah, I can't use 10min as default because it will affect how soon the data is available in Pinot. We will just keep on waiting for kafka to return records



-- 
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: commits-unsubscribe@pinot.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org