impl for bulkupdate api #121

ankitchoudhary111 · 2021-08-02T06:52:19Z

No description provided.

codecov · 2021-08-02T06:53:58Z

Codecov Report

Merging #121 (ba5b6c6) into main (e2ae441) will increase coverage by 0.09%.
The diff coverage is 77.77%.

@@             Coverage Diff              @@
##               main     #121      +/-   ##
============================================
+ Coverage     59.61%   59.71%   +0.09%     
- Complexity      292      303      +11     
============================================
  Files            39       39              
  Lines          2974     3043      +69     
  Branches        368      376       +8     
============================================
+ Hits           1773     1817      +44     
- Misses         1029     1053      +24     
- Partials        172      173       +1

Flag	Coverage Δ
integration	`59.71% <77.77%> (+0.09%)`	⬆️
unit	`40.63% <77.77%> (+0.89%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...e/entity/query/service/EntityQueryServiceImpl.java	`77.07% <77.77%> (+0.44%)`	⬆️
...query/service/client/EntityQueryServiceClient.java	`0.00% <0.00%> (-83.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2ae441...ba5b6c6. Read the comment docs.

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

skjindal93 · 2021-08-04T09:56:29Z

There are some existing review comments. Please take care of them as well. Github must have hidden them.

skjindal93 · 2021-08-04T09:57:35Z

Once the review comments are addressed, please also add the unit and integration tests.

aaron-steinfeld

There's a lot of resolved comments here that aren't actually resolved. I tried to clear out my comments that had already been mentioned, but I may have repeated some stuff.

In the future @ankitchoudhary111 - generally the commenter, not the author, is responsible for resolving comments once they review with the author's fix.

aaron-steinfeld · 2021-08-04T12:48:14Z

...lient/src/main/java/org/hypertrace/entity/query/service/client/EntityQueryServiceClient.java

@@ -35,6 +36,12 @@ public EntityQueryServiceClient(Channel channel) {
        headers, () -> blockingStub.update(updateRequest));
  }

+  public Iterator<ResultSetChunk> bulkUpdate(
+      BulkEntityUpdateRequest updateRequest, Map<String, String> headers) {


I would vote to not put this in the client (since I think we mostly use the native stubs now, anyway). Mainly, I don't want to spread the tech debt of this client in terms of exposing iterators and header maps.

aaron-steinfeld · 2021-08-04T12:52:28Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

@@ -290,19 +295,128 @@ private void doUpdate(RequestContext requestContext, EntityUpdateRequest request
          EntityQueryConverter.convertToAttributeValue(setAttribute.getValue()).build();
      String jsonValue = DocStoreJsonFormat.printer().print(attributeValue);

+      Map<Key, Map<String, Document>> entitiesUpdateMap = new HashMap<>();
+
      for (String entityId : request.getEntityIdsList()) {
        SingleValueKey key =
            new SingleValueKey(requestContext.getTenantId().orElseThrow(), entityId);
        // TODO better error reporting once doc store exposes the,


Still relevant with the change?

I am not even sure, what the current comment even means, because it's incomplete

aaron-steinfeld · 2021-08-04T12:53:15Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

@@ -290,19 +295,128 @@ private void doUpdate(RequestContext requestContext, EntityUpdateRequest request
          EntityQueryConverter.convertToAttributeValue(setAttribute.getValue()).build();
      String jsonValue = DocStoreJsonFormat.printer().print(attributeValue);

+      Map<Key, Map<String, Document>> entitiesUpdateMap = new HashMap<>();
+
      for (String entityId : request.getEntityIdsList()) {
        SingleValueKey key =
            new SingleValueKey(requestContext.getTenantId().orElseThrow(), entityId);
        // TODO better error reporting once doc store exposes the,


Still relevant with the change?

aaron-steinfeld · 2021-08-04T12:55:01Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

-              key,
-              subDocPath,
-              jsonValue);
+        if (entitiesUpdateMap.containsKey(key)) {


can clean up this if/else with computeIfAbsent

aaron-steinfeld · 2021-08-04T12:56:01Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

+      BulkEntityUpdateRequest request, StreamObserver<ResultSetChunk> responseObserver) {
+    // Validations
+    RequestContext requestContext = RequestContext.CURRENT.get();
+    Optional<String> tenantId = requestContext.getTenantId();


nit, orElse(null) this here so we don't have to optional.get() later (where we lose the context that this has been checked for presence)

aaron-steinfeld · 2021-08-04T12:58:01Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

        }
      }
+      try {
+        entitiesCollection.bulkUpdateSubDocs(entitiesUpdateMap);


The update method doesn't return? If not, can we make it? Seems silly (and inefficient) to have to update then query again to get the results

Couple of reasons

The bulkUpdateSubDocs method currently only returns the number of successfully updated documents.
We just tried to follow what the existing bulkUpdate function does for entities

Trying to follow what the existing update API does. I think the use case was just to avoid another GET entities query to get the projected selections

I'm really asking why bulkUpdateSubDocs doesn't, since it looks like at least here we need to do the following select/get anyway. Is it that the underlying store doesn't return this anyway, so even if we returned it from the api we're making two calls, an update and a select?

need to check the update support in the underlying store, but shouldn't the bulkUpdateSubDocs follow the same semantics as bulkUpdate?

Yes, would make the same argument there - they both return the same object, so extending that object with the actual updated documents would affect both APIs.

Yeah, we could extend that to return the actual updated documents, but I don't think we would be able to get the projected entities with just the requested selections in that case, unless we actually pass the set of requested projections to the document store API, and then ask it to return the projected documents (as part of the same request, if Mongo/Postgres can support that)

In fact, these are 2 APIs merged into one

bulk update entities

get projected entities (irrespective of the updated ones)

now, that I think about it, the API sounds weird. I think we should get rid of the projected selections from the bulk update API

So taking a glance at the impls, at least postgres (not sure about mongo) does not support this, so it would probably make sense to keep it as a distinct API - that decision should likely bubble up then so we're consistent - if entity service is forced to make two calls, then we may as well have the caller make the two calls since there are certainly cases where we don't care about the updated docs.

agreed.

Mongo does support that, but I would rather prefer to drop the projections support from the API, as you mentioned too

@ankitchoudhary111 Can you please get rid of the projections support from the bulk update API?

aaron-steinfeld · 2021-08-04T13:01:47Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

+      }
+    }
+
+    if (entitiesUpdateMap.isEmpty()) {


shouldn't really be reachable - there should never be a successful case where we return without actually doing an update.

aaron-steinfeld · 2021-08-04T13:02:31Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

+    try {
+      BulkUpdateResult bulkUpdateResult = entitiesCollection.bulkUpdateSubDocs(toUpdate);
+    } catch (Exception e) {
+      LOG.warn("Failed to update entities", e);


So if the update fails, we catch it, log a warning and proceed on successfully? That doesn't seem right.

skjindal93

Please add integration tests https://github.com/hypertrace/entity-service/blob/main/entity-service/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

…into updateapiimpl

skjindal93 · 2021-08-10T07:01:28Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

+        String jsonValue = DocStoreJsonFormat.printer().print(attributeValue);
+        documentMap.put(subDocPath, new JSONDocument(jsonValue));
+      } catch (Exception e) {
+        LOG.warn("Failed to put update corresponding to {} in the documentMap", subDocPath, e);


We should throw the exception back here

Suggested change

LOG.warn("Failed to put update corresponding to {} in the documentMap", subDocPath, e);

LOG.error("Failed to put update corresponding to {} in the documentMap", subDocPath, e);

skjindal93 · 2021-08-10T07:04:32Z

...rvice-impl/src/test/java/org/hypertrace/entity/query/service/EntityQueryServiceImplTest.java

+  }
+
+  @Test
+  public void testBulkUpdate_noTenantId() throws Exception {


Can you put all the bulk update tests inside a nested test class?

skjindal93 · 2021-08-10T07:04:49Z

...rvice-impl/src/test/java/org/hypertrace/entity/query/service/EntityQueryServiceImplTest.java

+              eqs.bulkUpdate(bulkUpdateRequest, mockResponseObserver);
+              return null;
+            });
+    verify(entitiesCollection, times(0)).bulkUpdateSubDocs(any());


Suggested change

verify(entitiesCollection, times(0)).bulkUpdateSubDocs(any());

verify(entitiesCollection, never()).bulkUpdateSubDocs(any());

skjindal93 · 2021-08-10T07:05:12Z

...e/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

@@ -68,7 +76,7 @@
  private static final Logger LOG = LoggerFactory.getLogger(EntityQueryServiceTest.class);
  private static final Slf4jLogConsumer logConsumer = new Slf4jLogConsumer(LOG);

-  private static EntityQueryServiceClient entityQueryServiceClient;
+  private static EntityQueryServiceBlockingStub blockingStub;


Suggested change

private static EntityQueryServiceBlockingStub blockingStub;

private static EntityQueryServiceBlockingStub entityQueryServiceClient;

skjindal93 · 2021-08-10T07:05:43Z

...e/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

@@ -82,6 +90,7 @@
  private static final String SERVICE_ID = generateRandomUUID();

  private static Map<String, String> apiAttributesMap;
+  private static Map<String, String> headers = Map.of("x-tenant-id", TENANT_ID);


statis final variables are upper cased

Suggested change

private static Map<String, String> headers = Map.of("x-tenant-id", TENANT_ID);

private static final Map<String, String> HEADERS = Map.of("x-tenant-id", TENANT_ID);

skjindal93 · 2021-08-10T07:06:34Z

...e/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

+            .putIdentifyingAttributes(
+                EntityConstants.getValue(ApiAttribute.API_ATTRIBUTE_API_TYPE),
+                createAttribute(API_TYPE));
+    ;


dangling semi colon

skjindal93 · 2021-08-10T07:06:44Z

...e/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

+            .putIdentifyingAttributes(
+                EntityConstants.getValue(ApiAttribute.API_ATTRIBUTE_API_TYPE),
+                createAttribute(API_TYPE));
+    ;


dangling semi colon

skjindal93 · 2021-08-10T07:07:03Z

...e/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

+            .putEntities(entity2.getEntityId(), updateInfo2)
+            .build();
+
+    Map<String, String> headers = Map.of("x-tenant-id", TENANT_ID);


We already have defined the headers at the class level. Can directly use them

...rvice-impl/src/test/java/org/hypertrace/entity/query/service/EntityQueryServiceImplTest.java

...e/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

skjindal93 · 2021-08-10T08:47:46Z

...e/src/integrationTest/java/org/hypertrace/entity/service/service/EntityQueryServiceTest.java

+            .putEntities(entity2.getEntityId(), updateInfo2)
+            .build();
+
+    Iterator<ResultSetChunk> resultChunkIterator =


Since, we aren't using resultChunkIterator, can we get rid of this?

skjindal93 · 2021-08-10T08:48:55Z

...y-service-impl/src/main/java/org/hypertrace/entity/query/service/EntityQueryServiceImpl.java

+        String jsonValue = DocStoreJsonFormat.printer().print(attributeValue);
+        documentMap.put(subDocPath, new JSONDocument(jsonValue));
+      } catch (Exception e) {
+        LOG.error("Failed to put update corresponding to {} in the documentMap", subDocPath, e);


Please throw the exception back #121 (comment)

…service/EntityQueryServiceImplTest.java Co-authored-by: SJ <48863181+skjindal93@users.noreply.github.com>

…service/service/EntityQueryServiceTest.java Co-authored-by: SJ <48863181+skjindal93@users.noreply.github.com>

github-actions · 2021-08-11T08:27:56Z

Unit Test Results

  31 files +1   31 suites +1 20s ⏱️ -3s
143 tests +6 143 ✔️ +6 0 💤 ±0 0 ❌ ±0

Results for commit 4cffb07. ± Comparison against base commit e2ae441.

This reverts commit 4cffb07.

impl for bulkupdate api

55d9f25

ankitchoudhary111 requested review from skjindal93, avinashkolluru and a team August 2, 2021 06:52

This comment has been minimized.

Sign in to view

skjindal93 reviewed Aug 4, 2021

View reviewed changes

resolved comments

3f449cc

This comment has been minimized.

Sign in to view

ankitchoudhary111 requested a review from skjindal93 August 4, 2021 08:57

skjindal93 reviewed Aug 4, 2021

View reviewed changes

..

078e11a

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Aug 4, 2021

View reviewed changes

Ankit Choudhary and others added 3 commits August 6, 2021 14:32

unit tests

54b6be1

...

afb6c1e

Merge branch 'main' into updateapiimpl

8d3472a

This comment has been minimized.

Sign in to view

skjindal93 reviewed Aug 9, 2021

View reviewed changes

Ankit Choudhary added 2 commits August 10, 2021 12:23

integration tests

b2ee5c5

Merge branch 'updateapiimpl' of github.com:hypertrace/entity-service …

da5e1bf

…into updateapiimpl

This comment has been minimized.

Sign in to view

ankitchoudhary111 requested a review from skjindal93 August 10, 2021 06:57

skjindal93 reviewed Aug 10, 2021

View reviewed changes

..

9568428

This comment has been minimized.

Sign in to view

ankitchoudhary111 requested a review from skjindal93 August 10, 2021 07:40

skjindal93 reviewed Aug 10, 2021

View reviewed changes

ankitchoudhary111 and others added 2 commits August 10, 2021 17:31

Update entity-service-impl/src/test/java/org/hypertrace/entity/query/…

8781aaf

…service/EntityQueryServiceImplTest.java Co-authored-by: SJ <48863181+skjindal93@users.noreply.github.com>

Update entity-service/src/integrationTest/java/org/hypertrace/entity/…

a156ffa

…service/service/EntityQueryServiceTest.java Co-authored-by: SJ <48863181+skjindal93@users.noreply.github.com>

This comment has been minimized.

Sign in to view

resolved

808e41a

This comment has been minimized.

Sign in to view

ankitchoudhary111 requested a review from skjindal93 August 10, 2021 12:11

skjindal93 approved these changes Aug 11, 2021

View reviewed changes

Merge branch 'main' into updateapiimpl

ba5b6c6

This comment has been minimized.

Sign in to view

ankitchoudhary111 merged commit 4cffb07 into main Aug 11, 2021

ankitchoudhary111 deleted the updateapiimpl branch August 11, 2021 08:25

skjindal93 added a commit that referenced this pull request Sep 2, 2021

Revert "impl for bulkupdate api (#121)"

ebab496

This reverts commit 4cffb07.

	LOG.warn("Failed to put update corresponding to {} in the documentMap", subDocPath, e);
	LOG.error("Failed to put update corresponding to {} in the documentMap", subDocPath, e);

	verify(entitiesCollection, times(0)).bulkUpdateSubDocs(any());
	verify(entitiesCollection, never()).bulkUpdateSubDocs(any());

	private static EntityQueryServiceBlockingStub blockingStub;
	private static EntityQueryServiceBlockingStub entityQueryServiceClient;

	private static Map<String, String> headers = Map.of("x-tenant-id", TENANT_ID);
	private static final Map<String, String> HEADERS = Map.of("x-tenant-id", TENANT_ID);

impl for bulkupdate api #121

impl for bulkupdate api #121

Uh oh!

Conversation

ankitchoudhary111 commented Aug 2, 2021

Uh oh!

codecov bot commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

skjindal93 commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjindal93 commented Aug 4, 2021

Uh oh!

This comment has been minimized.

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjindal93 Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjindal93 Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjindal93 Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

skjindal93 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 2, 2021 •

edited

Loading

skjindal93 commented Aug 4, 2021 •

edited

Loading

skjindal93 Aug 5, 2021 •

edited

Loading

skjindal93 Aug 5, 2021 •

edited

Loading

skjindal93 Aug 5, 2021 •

edited

Loading