You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/01/12 20:02:51 UTC

[GitHub] [beam] lukecwik opened a new pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

lukecwik opened a new pull request #16495:
URL: https://github.com/apache/beam/pull/16495


   
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   `ValidatesRunner` compliance status (on master branch)
   --------------------------------------------------------
   
   <table>
     <thead>
       <tr>
         <th>Lang</th>
         <th>ULR</th>
         <th>Dataflow</th>
         <th>Flink</th>
         <th>Samza</th>
         <th>Spark</th>
         <th>Twister2</th>
       </tr>
     </thead>
     <tbody>
       <tr>
         <td>Go</td>
         <td>---</td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
           </a>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Samza/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Samza/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>---</td>
       </tr>
       <tr>
         <td>Java</td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/badge/icon?subject=V1+Streaming">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon?subject=V1+Java+11">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/badge/icon?subject=V2+Streaming">
           </a><br>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon?subject=Java+8">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon?subject=Java+11">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon?subject=Portable">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon?subject=Portable+Streaming">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/badge/icon?subject=Portable">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon?subject=Portable">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon?subject=Structured+Streaming">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon">
           </a>
         </td>
       </tr>
       <tr>
         <td>Python</td>
         <td>---</td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon?subject=ValCont">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon?subject=Portable">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Samza/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Samza/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>---</td>
       </tr>
       <tr>
         <td>XLang</td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Samza/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Samza/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>---</td>
       </tr>
     </tbody>
   </table>
   
   Examples testing status on various runners
   --------------------------------------------------------
   
   <table>
     <thead>
       <tr>
         <th>Lang</th>
         <th>ULR</th>
         <th>Dataflow</th>
         <th>Flink</th>
         <th>Samza</th>
         <th>Spark</th>
         <th>Twister2</th>
       </tr>
     </thead>
     <tbody>
       <tr>
         <td>Go</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
       </tr>
       <tr>
         <td>Java</td>
         <td>---</td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/badge/icon?subject=V1">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/badge/icon?subject=V1+Java11">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
           </a><br>
         </td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
       </tr>
       <tr>
         <td>Python</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
       </tr>
       <tr>
         <td>XLang</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
       </tr>
     </tbody>
   </table>
   
   Post-Commit SDK/Transform Integration Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   <table>
     <thead>
       <tr>
         <th>Go</th>
         <th>Java</th>
         <th>Python</th>
       </tr>
     </thead>
     <tbody>
       <tr>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon?subject=3.6">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon?subject=3.7">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon?subject=3.8">
           </a>
         </td>
       </tr>
     </tbody>
   </table>
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   <table>
     <thead>
       <tr>
         <th>---</th>
         <th>Java</th>
         <th>Python</th>
         <th>Go</th>
         <th>Website</th>
         <th>Whitespace</th>
         <th>Typescript</th>
       </tr>
     </thead>
     <tbody>
       <tr>
         <td>Non-portable</td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon">
           </a><br>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon?subject=Tests">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon?subject=Lint">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon?subject=Docker">
           </a><br>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon?subject=Docs">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon">
           </a>
         </td>
       </tr>
       <tr>
         <td>Portable</td>
         <td>---</td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>
           <a href="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/">
             <img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/badge/icon">
           </a>
         </td>
         <td>---</td>
         <td>---</td>
         <td>---</td>
       </tr>
     </tbody>
   </table>
   
   See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
   
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #16495:
URL: https://github.com/apache/beam/pull/16495#issuecomment-1011405211


   R: @emilymye @scwhittle 


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #16495:
URL: https://github.com/apache/beam/pull/16495#issuecomment-1015682070


   PTAL


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r786378958



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -166,6 +165,10 @@ public int weigh(Object key, WeightedValue<Object> value) {
                             return (int) size;
                           }
                         })
+                    // The maximum size of an entry in the cache is maxWeight / concurrencyLevel
+                    // which is why we set the concurrency level to 1. See
+                    // https://github.com/google/guava/issues/3462 for further details.
+                    .concurrencyLevel(1)

Review comment:
       I have a benchmark that I've been meaning to push up after this change that compared with and without cross bundle caching. I used it to check and it looks like there is no noticeable difference.
   
   With the concurrency level at the default (4):
   ```
   Result "org.apache.beam.fn.harness.jmh.ProcessBundleBenchmark.testStateWithCaching":
     4452.734 ±(99.9%) 60.263 ops/s [Average]
     (min, avg, max) = (4227.619, 4452.734, 4550.040), stdev = 80.449
     CI (99.9%): [4392.471, 4512.997] (assumes normal distribution)
   ```
   
   With the concurrent level at the 1:
   ```
   Result "org.apache.beam.fn.harness.jmh.ProcessBundleBenchmark.testStateWithCaching":
     4445.516 ±(99.9%) 41.809 ops/s [Average]
     (min, avg, max) = (4345.263, 4445.516, 4536.817), stdev = 55.814
     CI (99.9%): [4403.707, 4487.325] (assumes normal distribution)
   ```




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #16495:
URL: https://github.com/apache/beam/pull/16495#issuecomment-1011448277


   Run Java_PVR_Flink_Docker PreCommit


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] scwhittle commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
scwhittle commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r785784865



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -166,6 +165,10 @@ public int weigh(Object key, WeightedValue<Object> value) {
                             return (int) size;
                           }
                         })
+                    // The maximum size of an entry in the cache is maxWeight / concurrencyLevel
+                    // which is why we set the concurrency level to 1. See
+                    // https://github.com/google/guava/issues/3462 for further details.
+                    .concurrencyLevel(1)

Review comment:
       Would be good to verify this setting doesn't add additional contention and add that justification to the comment.
   
   If it is worse (are worker threads modifying this directly?) then perhaps it would be better to keep the concurrency level and just update option comment about maximum element size or log what the maximum size for element can be.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r785177482



##########
File path: sdks/java/harness/src/test/java/org/apache/beam/fn/harness/CachesTest.java
##########
@@ -160,4 +160,20 @@ private void testCache(Cache<String, String> cache) {
     assertEquals("value2", cache.computeIfAbsent("key2", (unused) -> "value2"));
     assertEquals("value2", cache.peek("key2"));
   }
+
+  @Test
+  public void testDescribeStats() throws Exception {
+    Cache<Integer, WeightedValue<String>> cache = Caches.forMaximumBytes(1000 * 1048576L);
+    for (int i = 0; i < 100; ++i) {
+      cache.computeIfAbsent(i, (key) -> WeightedValue.of("value", 1048576L));
+      cache.peek(i);
+      cache.put(100 + i, WeightedValue.of("value", 1048576L));
+    }
+
+    assertThat(cache.describeStats(), containsString("used/max 200/1000 MB"));

Review comment:
       Added eviction count and checked evictions and shrinkage.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #16495:
URL: https://github.com/apache/beam/pull/16495#issuecomment-1013479260


   PTAL


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik edited a comment on pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik edited a comment on pull request #16495:
URL: https://github.com/apache/beam/pull/16495#issuecomment-1015946680


   PTAL
   
   Also, if we want to make this into the 2.36 release it would make sense to merge before the branch is recut (likely tomorrow).


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r786406745



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -166,6 +165,10 @@ public int weigh(Object key, WeightedValue<Object> value) {
                             return (int) size;
                           }
                         })
+                    // The maximum size of an entry in the cache is maxWeight / concurrencyLevel
+                    // which is why we set the concurrency level to 1. See
+                    // https://github.com/google/guava/issues/3462 for further details.
+                    .concurrencyLevel(1)

Review comment:
       State caching benchmark in https://github.com/apache/beam/pull/16542




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r785134812



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -57,92 +65,58 @@ public static long weigh(Object o) {
 
   /** An eviction listener that reduces the size of entries that are {@link Shrinkable}. */
   @VisibleForTesting
-  static class ShrinkOnEviction implements RemovalListener<CompositeKey, Object> {
+  static class ShrinkOnEviction implements RemovalListener<CompositeKey, WeightedValue<Object>> {
 
     private final org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<
-            CompositeKey, Object>
+            CompositeKey, WeightedValue<Object>>
         cache;
+    private final LongAdder weightInBytes;
 
-    ShrinkOnEviction(CacheBuilder<Object, Object> cacheBuilder) {
+    ShrinkOnEviction(
+        CacheBuilder<Object, WeightedValue<Object>> cacheBuilder, LongAdder weightInBytes) {
       this.cache = cacheBuilder.removalListener(this).build();
+      this.weightInBytes = weightInBytes;
     }
 
     public org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<
-            CompositeKey, Object>
+            CompositeKey, WeightedValue<Object>>
         getCache() {
       return cache;
     }
 
     @Override
-    public void onRemoval(RemovalNotification<CompositeKey, Object> removalNotification) {
+    public void onRemoval(
+        RemovalNotification<CompositeKey, WeightedValue<Object>> removalNotification) {
+      weightInBytes.add(-removalNotification.getValue().getWeight());
       if (removalNotification.wasEvicted()) {
-        if (!(removalNotification.getValue() instanceof Cache.Shrinkable)) {
+        if (!(removalNotification.getValue().getValue() instanceof Cache.Shrinkable)) {
           return;
         }
-        Object updatedEntry = ((Shrinkable<?>) removalNotification.getValue()).shrink();
+        Object updatedEntry = ((Shrinkable<?>) removalNotification.getValue().getValue()).shrink();
         if (updatedEntry != null) {
-          cache.put(removalNotification.getKey(), updatedEntry);
+          cache.put(removalNotification.getKey(), addWeightedValue(updatedEntry, weightInBytes));
         }
       }
     }
   }
 
   /** A cache that never stores any values. */
   public static <K, V> Cache<K, V> noop() {
-    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
-    // preventing deadlock from occurring when a loading function mutates the underlying cache
-    return (Cache<K, V>)
-        forCache(new ShrinkOnEviction(CacheBuilder.newBuilder().maximumSize(0)).getCache());
+    return forMaximumBytes(0L);
   }
 
   /** A cache that never evicts any values. */
   public static <K, V> Cache<K, V> eternal() {
-    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
-    // preventing deadlock from occurring when a loading function mutates the underlying cache
-    return (Cache<K, V>)
-        forCache(
-            new ShrinkOnEviction(CacheBuilder.newBuilder().maximumSize(Long.MAX_VALUE)).getCache());
+    return forMaximumBytes(Long.MAX_VALUE);
   }
 
   /**
    * Uses the specified {@link PipelineOptions} to configure and return a cache instance based upon
    * parameters within {@link SdkHarnessOptions}.
    */
   public static <K, V> Cache<K, V> fromOptions(PipelineOptions options) {
-    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
-    // preventing deadlock from occurring when a loading function mutates the underlying cache
-    return (Cache<K, V>)
-        forCache(
-            new ShrinkOnEviction(
-                    CacheBuilder.newBuilder()
-                        .maximumWeight(
-                            options.as(SdkHarnessOptions.class).getMaxCacheMemoryUsageMb()
-                                * 1024L
-                                * 1024L
-                                / WEIGHT_RATIO)
-                        .weigher(
-                            new Weigher<Object, Object>() {
-
-                              @Override
-                              public int weigh(Object key, Object value) {
-                                long size;
-                                if (value instanceof Weighted) {
-                                  size = Caches.weigh(key) + ((Weighted) value).getWeight();
-                                } else {
-                                  size = Caches.weigh(key) + Caches.weigh(value);
-                                }
-                                size = size / WEIGHT_RATIO + 1;
-                                if (size >= Integer.MAX_VALUE) {
-                                  LOG.warn(
-                                      "Entry with size {} MiBs inserted into the cache. This is larger than the maximum individual entry size of {} MiBs. The cache will under report its memory usage by the difference. This may lead to OutOfMemoryErrors.",
-                                      (size / 1048576L) + 1,
-                                      2 * WEIGHT_RATIO * 1024);
-                                  return Integer.MAX_VALUE;
-                                }
-                                return (int) size;
-                              }
-                            }))
-                .getCache());
+    return forMaximumBytes(
+        options.as(SdkHarnessOptions.class).getMaxCacheMemoryUsageMb() * 1048576L);

Review comment:
       swapped to bit shift.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik merged pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik merged pull request #16495:
URL: https://github.com/apache/beam/pull/16495


   


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r787248109



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -165,10 +141,57 @@ public int weigh(Object key, Object value) {
   }
 
   @VisibleForTesting
-  static Cache<Object, Object> forCache(
-      org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<CompositeKey, Object>
-          cache) {
-    return new SubCache<>(cache, CompositeKeyPrefix.ROOT);
+  static <K, V> Cache<K, V> forMaximumBytes(long maximumBytes) {
+    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
+    // preventing deadlock from occurring when a loading function mutates the underlying cache
+    LongAdder weightInBytes = new LongAdder();
+    return new SubCache<>(
+        new ShrinkOnEviction(
+                CacheBuilder.newBuilder()
+                    .maximumWeight(maximumBytes >> WEIGHT_RATIO)
+                    .weigher(
+                        new Weigher<Object, WeightedValue<Object>>() {
+
+                          @Override
+                          public int weigh(Object key, WeightedValue<Object> value) {
+                            // Round up to the next closest multiple of WEIGHT_RATIO
+                            long size = ((value.getWeight() - 1) >> WEIGHT_RATIO) + 1;

Review comment:
       I thought it was going to be a hassle to add when I was re-implementing it so we wouldn't have to weigh the key again when removing the entry but it turned out easier then I thought.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r787224006



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -165,10 +141,57 @@ public int weigh(Object key, Object value) {
   }
 
   @VisibleForTesting
-  static Cache<Object, Object> forCache(
-      org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<CompositeKey, Object>
-          cache) {
-    return new SubCache<>(cache, CompositeKeyPrefix.ROOT);
+  static <K, V> Cache<K, V> forMaximumBytes(long maximumBytes) {
+    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
+    // preventing deadlock from occurring when a loading function mutates the underlying cache
+    LongAdder weightInBytes = new LongAdder();
+    return new SubCache<>(
+        new ShrinkOnEviction(
+                CacheBuilder.newBuilder()
+                    .maximumWeight(maximumBytes >> WEIGHT_RATIO)
+                    .weigher(
+                        new Weigher<Object, WeightedValue<Object>>() {
+
+                          @Override
+                          public int weigh(Object key, WeightedValue<Object> value) {
+                            // Round up to the next closest multiple of WEIGHT_RATIO
+                            long size = ((value.getWeight() - 1) >> WEIGHT_RATIO) + 1;

Review comment:
       Originally I was having trouble getting a good way of not paying the cost of weighing the key over and over but I solved that and added it back.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] scwhittle commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
scwhittle commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r787108590



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -166,6 +165,10 @@ public int weigh(Object key, WeightedValue<Object> value) {
                             return (int) size;
                           }
                         })
+                    // The maximum size of an entry in the cache is maxWeight / concurrencyLevel
+                    // which is why we set the concurrency level to 1. See
+                    // https://github.com/google/guava/issues/3462 for further details.
+                    .concurrencyLevel(1)

Review comment:
       Thanks!




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r787224006



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -165,10 +141,57 @@ public int weigh(Object key, Object value) {
   }
 
   @VisibleForTesting
-  static Cache<Object, Object> forCache(
-      org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<CompositeKey, Object>
-          cache) {
-    return new SubCache<>(cache, CompositeKeyPrefix.ROOT);
+  static <K, V> Cache<K, V> forMaximumBytes(long maximumBytes) {
+    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
+    // preventing deadlock from occurring when a loading function mutates the underlying cache
+    LongAdder weightInBytes = new LongAdder();
+    return new SubCache<>(
+        new ShrinkOnEviction(
+                CacheBuilder.newBuilder()
+                    .maximumWeight(maximumBytes >> WEIGHT_RATIO)
+                    .weigher(
+                        new Weigher<Object, WeightedValue<Object>>() {
+
+                          @Override
+                          public int weigh(Object key, WeightedValue<Object> value) {
+                            // Round up to the next closest multiple of WEIGHT_RATIO
+                            long size = ((value.getWeight() - 1) >> WEIGHT_RATIO) + 1;

Review comment:
       Originally I was having trouble getting a good way of not paying the cost of weighing the key over and over but I solved that and added it back.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] scwhittle commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
scwhittle commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r783999225



##########
File path: sdks/java/harness/src/test/java/org/apache/beam/fn/harness/CachesTest.java
##########
@@ -160,4 +160,20 @@ private void testCache(Cache<String, String> cache) {
     assertEquals("value2", cache.computeIfAbsent("key2", (unused) -> "value2"));
     assertEquals("value2", cache.peek("key2"));
   }
+
+  @Test
+  public void testDescribeStats() throws Exception {
+    Cache<Integer, WeightedValue<String>> cache = Caches.forMaximumBytes(1000 * 1048576L);
+    for (int i = 0; i < 100; ++i) {
+      cache.computeIfAbsent(i, (key) -> WeightedValue.of("value", 1048576L));
+      cache.peek(i);
+      cache.put(100 + i, WeightedValue.of("value", 1048576L));
+    }
+
+    assertThat(cache.describeStats(), containsString("used/max 200/1000 MB"));

Review comment:
       would be good to have some test of the correctness of the used bytes as item weight is updated or evictions occur

##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/util/WeightedValue.java
##########
@@ -47,4 +49,26 @@ public long getWeight() {
   public T getValue() {
     return value;
   }
+
+  @Override
+  public boolean equals(@Nullable Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (!(o instanceof WeightedValue)) {

Review comment:
       check for null?

##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -57,92 +65,58 @@ public static long weigh(Object o) {
 
   /** An eviction listener that reduces the size of entries that are {@link Shrinkable}. */
   @VisibleForTesting
-  static class ShrinkOnEviction implements RemovalListener<CompositeKey, Object> {
+  static class ShrinkOnEviction implements RemovalListener<CompositeKey, WeightedValue<Object>> {
 
     private final org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<
-            CompositeKey, Object>
+            CompositeKey, WeightedValue<Object>>
         cache;
+    private final LongAdder weightInBytes;
 
-    ShrinkOnEviction(CacheBuilder<Object, Object> cacheBuilder) {
+    ShrinkOnEviction(
+        CacheBuilder<Object, WeightedValue<Object>> cacheBuilder, LongAdder weightInBytes) {
       this.cache = cacheBuilder.removalListener(this).build();
+      this.weightInBytes = weightInBytes;
     }
 
     public org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<
-            CompositeKey, Object>
+            CompositeKey, WeightedValue<Object>>
         getCache() {
       return cache;
     }
 
     @Override
-    public void onRemoval(RemovalNotification<CompositeKey, Object> removalNotification) {
+    public void onRemoval(
+        RemovalNotification<CompositeKey, WeightedValue<Object>> removalNotification) {
+      weightInBytes.add(-removalNotification.getValue().getWeight());
       if (removalNotification.wasEvicted()) {
-        if (!(removalNotification.getValue() instanceof Cache.Shrinkable)) {
+        if (!(removalNotification.getValue().getValue() instanceof Cache.Shrinkable)) {
           return;
         }
-        Object updatedEntry = ((Shrinkable<?>) removalNotification.getValue()).shrink();
+        Object updatedEntry = ((Shrinkable<?>) removalNotification.getValue().getValue()).shrink();
         if (updatedEntry != null) {
-          cache.put(removalNotification.getKey(), updatedEntry);
+          cache.put(removalNotification.getKey(), addWeightedValue(updatedEntry, weightInBytes));
         }
       }
     }
   }
 
   /** A cache that never stores any values. */
   public static <K, V> Cache<K, V> noop() {
-    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
-    // preventing deadlock from occurring when a loading function mutates the underlying cache
-    return (Cache<K, V>)
-        forCache(new ShrinkOnEviction(CacheBuilder.newBuilder().maximumSize(0)).getCache());
+    return forMaximumBytes(0L);
   }
 
   /** A cache that never evicts any values. */
   public static <K, V> Cache<K, V> eternal() {
-    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
-    // preventing deadlock from occurring when a loading function mutates the underlying cache
-    return (Cache<K, V>)
-        forCache(
-            new ShrinkOnEviction(CacheBuilder.newBuilder().maximumSize(Long.MAX_VALUE)).getCache());
+    return forMaximumBytes(Long.MAX_VALUE);
   }
 
   /**
    * Uses the specified {@link PipelineOptions} to configure and return a cache instance based upon
    * parameters within {@link SdkHarnessOptions}.
    */
   public static <K, V> Cache<K, V> fromOptions(PipelineOptions options) {
-    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
-    // preventing deadlock from occurring when a loading function mutates the underlying cache
-    return (Cache<K, V>)
-        forCache(
-            new ShrinkOnEviction(
-                    CacheBuilder.newBuilder()
-                        .maximumWeight(
-                            options.as(SdkHarnessOptions.class).getMaxCacheMemoryUsageMb()
-                                * 1024L
-                                * 1024L
-                                / WEIGHT_RATIO)
-                        .weigher(
-                            new Weigher<Object, Object>() {
-
-                              @Override
-                              public int weigh(Object key, Object value) {
-                                long size;
-                                if (value instanceof Weighted) {
-                                  size = Caches.weigh(key) + ((Weighted) value).getWeight();
-                                } else {
-                                  size = Caches.weigh(key) + Caches.weigh(value);
-                                }
-                                size = size / WEIGHT_RATIO + 1;
-                                if (size >= Integer.MAX_VALUE) {
-                                  LOG.warn(
-                                      "Entry with size {} MiBs inserted into the cache. This is larger than the maximum individual entry size of {} MiBs. The cache will under report its memory usage by the difference. This may lead to OutOfMemoryErrors.",
-                                      (size / 1048576L) + 1,
-                                      2 * WEIGHT_RATIO * 1024);
-                                  return Integer.MAX_VALUE;
-                                }
-                                return (int) size;
-                              }
-                            }))
-                .getCache());
+    return forMaximumBytes(
+        options.as(SdkHarnessOptions.class).getMaxCacheMemoryUsageMb() * 1048576L);

Review comment:
       nit: I think 1024* 1024 is easier to eyeball correctness




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] scwhittle commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
scwhittle commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r787112419



##########
File path: sdks/java/harness/src/main/java/org/apache/beam/fn/harness/Caches.java
##########
@@ -165,10 +141,57 @@ public int weigh(Object key, Object value) {
   }
 
   @VisibleForTesting
-  static Cache<Object, Object> forCache(
-      org.apache.beam.vendor.guava.v26_0_jre.com.google.common.cache.Cache<CompositeKey, Object>
-          cache) {
-    return new SubCache<>(cache, CompositeKeyPrefix.ROOT);
+  static <K, V> Cache<K, V> forMaximumBytes(long maximumBytes) {
+    // We specifically use Guava cache since it allows for recursive computeIfAbsent calls
+    // preventing deadlock from occurring when a loading function mutates the underlying cache
+    LongAdder weightInBytes = new LongAdder();
+    return new SubCache<>(
+        new ShrinkOnEviction(
+                CacheBuilder.newBuilder()
+                    .maximumWeight(maximumBytes >> WEIGHT_RATIO)
+                    .weigher(
+                        new Weigher<Object, WeightedValue<Object>>() {
+
+                          @Override
+                          public int weigh(Object key, WeightedValue<Object> value) {
+                            // Round up to the next closest multiple of WEIGHT_RATIO
+                            long size = ((value.getWeight() - 1) >> WEIGHT_RATIO) + 1;

Review comment:
       It seems like we used to have the cache weigh the key with Caches.weigh(key) but I no longer see that happening.  Was that intentional?
   
   If not, add a test where there are large keys to verify that they are weighed.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on pull request #16495:
URL: https://github.com/apache/beam/pull/16495#issuecomment-1015946680


   PTAL


-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] lukecwik commented on a change in pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #16495:
URL: https://github.com/apache/beam/pull/16495#discussion_r785104313



##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/util/WeightedValue.java
##########
@@ -47,4 +49,26 @@ public long getWeight() {
   public T getValue() {
     return value;
   }
+
+  @Override
+  public boolean equals(@Nullable Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (!(o instanceof WeightedValue)) {

Review comment:
       `null instanceof X` is always false so checking for null is redundant.




-- 
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: github-unsubscribe@beam.apache.org

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



[GitHub] [beam] scwhittle commented on pull request #16495: [BEAM-13015] Provide caching statistics in the status client.

Posted by GitBox <gi...@apache.org>.
scwhittle commented on pull request #16495:
URL: https://github.com/apache/beam/pull/16495#issuecomment-1016315974


   LGTM thanks!


-- 
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: github-unsubscribe@beam.apache.org

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