You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "pabloem (via GitHub)" <gi...@apache.org> on 2023/03/03 01:11:33 UTC

[GitHub] [beam] pabloem opened a new pull request, #25701: Fix DatastoreIO bug causing low throughput on certain writes

pabloem opened a new pull request, #25701:
URL: https://github.com/apache/beam/pull/25701

   **Please** add a meaningful description for your change here
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
    - [ ] 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/get-started-contributing/#make-the-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   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)
   [![Go tests](https://github.com/apache/beam/workflows/Go%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Go+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] github-actions[bot] commented on pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25701:
URL: https://github.com/apache/beam/pull/25701#issuecomment-1452808138

   Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control


-- 
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] Abacn commented on a diff in pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25701:
URL: https://github.com/apache/beam/pull/25701#discussion_r1128655747


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java:
##########
@@ -1542,12 +1542,27 @@ public void startBundle(StartBundleContext c) {
       }
     }
 
+    private static com.google.datastore.v1.Key getKey(Mutation m) {
+      if (m.hasUpsert()) {
+        return m.getUpsert().getKey();
+      } else if (m.hasInsert()) {
+        return m.getInsert().getKey();
+      } else if (m.hasDelete()) {
+        return m.getDelete();
+      } else if (m.hasUpdate()) {
+        return m.getUpdate().getKey();
+      } else {
+        LOG.warning("Mutation %s does not have an operation type set.", m);

Review Comment:
   ```suggestion
           LOG.warn("Mutation {} does not have an operation type set.", m);
   ```



-- 
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] pabloem commented on a diff in pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "pabloem (via GitHub)" <gi...@apache.org>.
pabloem commented on code in PR #25701:
URL: https://github.com/apache/beam/pull/25701#discussion_r1128268864


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java:
##########
@@ -1542,12 +1542,26 @@ public void startBundle(StartBundleContext c) {
       }
     }
 
+    private static com.google.datastore.v1.Key getKey(Mutation m) {
+      if (m.hasUpsert()) {
+        return m.getUpsert().getKey();
+      } else if (m.hasInsert()) {
+        return m.getInsert().getKey();
+      } else if (m.hasDelete()) {
+        return m.getDelete();
+      } else if (m.hasUpdate()) {
+        return m.getUpdate().getKey();
+      } else {
+        throw new IllegalArgumentException(String.format("Mutation %s is not valid.", m));

Review Comment:
   ok I've done that. Thanks Yi!



-- 
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] Abacn commented on a diff in pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25701:
URL: https://github.com/apache/beam/pull/25701#discussion_r1128284597


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java:
##########
@@ -1542,12 +1542,27 @@ public void startBundle(StartBundleContext c) {
       }
     }
 
+    private static com.google.datastore.v1.Key getKey(Mutation m) {
+      if (m.hasUpsert()) {
+        return m.getUpsert().getKey();
+      } else if (m.hasInsert()) {
+        return m.getInsert().getKey();
+      } else if (m.hasDelete()) {
+        return m.getDelete();
+      } else if (m.hasUpdate()) {
+        return m.getUpdate().getKey();
+      } else {
+        LOG.warning("Mutation %s does not have an operation type set.", m);
+        return "";

Review Comment:
   ah need to return a Entity.getDefaultInstance().getKey()



-- 
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] pabloem commented on pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "pabloem (via GitHub)" <gi...@apache.org>.
pabloem commented on PR #25701:
URL: https://github.com/apache/beam/pull/25701#issuecomment-1452816564

   Run Java 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] Abacn commented on a diff in pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25701:
URL: https://github.com/apache/beam/pull/25701#discussion_r1128284597


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java:
##########
@@ -1542,12 +1542,27 @@ public void startBundle(StartBundleContext c) {
       }
     }
 
+    private static com.google.datastore.v1.Key getKey(Mutation m) {
+      if (m.hasUpsert()) {
+        return m.getUpsert().getKey();
+      } else if (m.hasInsert()) {
+        return m.getInsert().getKey();
+      } else if (m.hasDelete()) {
+        return m.getDelete();
+      } else if (m.hasUpdate()) {
+        return m.getUpdate().getKey();
+      } else {
+        LOG.warning("Mutation %s does not have an operation type set.", m);
+        return "";

Review Comment:
   ah need to return a Key.getDefaultInstance()



-- 
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] pabloem commented on pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "pabloem (via GitHub)" <gi...@apache.org>.
pabloem commented on PR #25701:
URL: https://github.com/apache/beam/pull/25701#issuecomment-1452817426

   > Thanks for the fix. What was the problem here? I sit excessive flush?
   
   It's the opposite - when the mutation was not of `upsert` type, it was causing flushing every two mutations (because `mutation.getUpsert().getKey()` always returns an empty string for non-upsert mutations.


-- 
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] pabloem merged pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

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


-- 
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] pabloem commented on pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "pabloem (via GitHub)" <gi...@apache.org>.
pabloem commented on PR #25701:
URL: https://github.com/apache/beam/pull/25701#issuecomment-1452796048

   r: @Abacn 


-- 
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] Abacn commented on a diff in pull request #25701: Fix DatastoreIO bug causing low throughput on certain writes

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #25701:
URL: https://github.com/apache/beam/pull/25701#discussion_r1123994353


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/datastore/DatastoreV1.java:
##########
@@ -1542,12 +1542,26 @@ public void startBundle(StartBundleContext c) {
       }
     }
 
+    private static com.google.datastore.v1.Key getKey(Mutation m) {
+      if (m.hasUpsert()) {
+        return m.getUpsert().getKey();
+      } else if (m.hasInsert()) {
+        return m.getInsert().getKey();
+      } else if (m.hasDelete()) {
+        return m.getDelete();
+      } else if (m.hasUpdate()) {
+        return m.getUpdate().getKey();
+      } else {
+        throw new IllegalArgumentException(String.format("Mutation %s is not valid.", m));

Review Comment:
   Are these 4 cases exhaustive? I also see `OPERATION_NOT_SET` in Mutation's source code. Maybe just add a warning and return an empty Key? Otherwise LGTM.



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