[OBSOLETE] Added API handler functions (scan and get) for LRO #15408

adrikagupta · 2023-11-07T12:52:07Z

Added scanOperations and getOperationRun API handler functions in OperationHttpHandler.java
Added getOperationRun() in OperationLifecycleManager.java
Added unit tests in the OperationLifecycleManagerTest.java
Manual testing for OperationHttpHandler is in progress

samdgupi · 2023-11-07T17:11:35Z

.vscode/launch.json

Please add .vscode to local gitignore so that this files are not committed

samdgupi · 2023-11-07T17:13:51Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+        responder,
+        OPERATIONS_LIST_PAGINATED_KEY,
+        jsonListResponder -> {
+          AtomicReference<OperationRun> lastRun = new AtomicReference<>(null);


no need to pass null

samdgupi · 2023-11-07T17:29:09Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    if (filter != null && !filter.isEmpty()) {
+      OperationType operationType = null;
+      OperationRunStatus operationStatus = null;
+      Pattern typePattern = Pattern.compile("type=(\\w+)");


Can we please convert this to a function which parses pattern of key=val(ANDkey=val)* then returns the key,val pairs. Then we can match the key to type/status using switch case. if there is a unknown key we throw exception.

samdgupi · 2023-11-07T17:29:42Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+        operationType = OperationType.valueOf(typeStr.toUpperCase());
+      }
+      while (statusMatcher.find()) {
+        String statusStr = statusMatcher.group(1);


throw error if not valid OperationStatus/OperationType

samdgupi · 2023-11-07T17:30:11Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    return builder.build();
+  }
+
+  private void validateNamespace(@Nullable String namespaceId) throws BadRequestException {


nit: move after public methods

samdgupi · 2023-11-07T17:32:59Z

cdap-app-fabric/src/main/java/io/cdap/cdap/internal/operation/OperationLifecycleManager.java

-/**
- * Service that manages lifecycle of Operation.
- */
+/** Service that manages lifecycle of Operation. */


Please remove all formatting changes

samdgupi · 2023-11-07T17:33:47Z

cdap-app-fabric/src/main/java/io/cdap/cdap/internal/operation/OperationLifecycleManager.java

 public class OperationLifecycleManager {

  private final TransactionRunner transactionRunner;
+  private static final AtomicInteger sourceId = new AtomicInteger();


why this are added ? this are not being used anywhere

My bad. That time I was trying to do manual testing but forgot to remove it later. I'll double check before pushing the changes

samdgupi · 2023-11-07T17:35:27Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

-    responder.sendJson(HttpResponseStatus.OK, GSON.toJson(runs));
+      @QueryParam("filter") String filter)
+      throws BadRequestException, IOException {
+    validateNamespace(namespaceId);


Add a todo for rbac check with a corresponding jira

Also, I think these handlers should have a feature flag check.

Got it. I have added the check for this feature flag: SOURCE_CONTROL_MANAGEMENT_MULTI_APP

samdgupi · 2023-11-07T17:38:50Z

cdap-app-fabric/src/test/java/io/cdap/cdap/internal/app/services/http/AppFabricTestBase.java

+
+    // if (latestOnly != null) {
+    //   uri += ("&latestOnly=" + latestOnly);
+    // }


remove commented lines

Right. done

samdgupi · 2023-11-07T17:40:15Z

...-app-fabric/src/test/java/io/cdap/cdap/internal/operation/OperationLifecycleManagerTest.java

+    return detail;
+  }
+
+  private static List<OperationRunDetail> insertTestRuns() throws Exception {


this is same in store also. we can separate this into a OperationTestBase class which in turn extends AppFabricTestBase

Done. Thank you for the suggestion :)

…entity token exchanges, and do not retry on status code zero for ApiExceptions

[CDAP-20879] Use default k8s namespace in hybrid mode for workload identity token exchanges

[CDAP-20785] do not set WI env vars when NSA is enabled

CDAP-20809: Operation Runner

Multi push operation

[CDAP-20785] Use GCP remote authenticator to fetch token

…o boolean

GnsP · 2023-11-09T09:21:05Z

.vscode/launch.json

@@ -0,0 +1,364 @@
+{


Please remove the .vscode folder from the PR.

GnsP · 2023-11-09T09:27:08Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

-    responder.sendJson(HttpResponseStatus.OK, GSON.toJson(runs));
+      @QueryParam("filter") String filter)
+      throws BadRequestException, IOException {
+    validateNamespace(namespaceId);


Also, I think these handlers should have a feature flag check.

GnsP · 2023-11-09T09:33:41Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+  private void validateNamespace(@Nullable String namespaceId) throws BadRequestException {
+    if (namespaceId == null) {
+      throw new BadRequestException("Path parameter namespaceId cannot be empty");
+    }
+  }


There are more cases that need to be validated here, like in this method.

Got it. I have added more cases

GnsP · 2023-11-09T09:38:40Z

...-app-fabric/src/test/java/io/cdap/cdap/internal/operation/OperationLifecycleManagerTest.java

+
+  @ClassRule public static final TemporaryFolder TEMP_FOLDER = new TemporaryFolder();
+
+  private static EmbeddedPostgres pg;


Please rename this variable for better readability. Possibly postgres or psql.

Sure. Thank you for the correction

Operation state publisher

Fix the null pointer exception caused by unsafe unboxing of Boolean

samdgupi · 2023-11-09T13:04:56Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    return new OperationRunFilter(operationType, operationStatus);
+  }
+
+  private static Map<String, String> parseFilter(String filter) {


nit: change the name to parseFilterStr

Yes done. I have edited this function and made it more generic

samdgupi · 2023-11-09T13:05:31Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    OperationRunStatus operationStatus = null;
+
+    for (Map.Entry<String, String> entry : filterKeyValMap.entrySet()) {
+      String key = entry.getKey();


samdgupi · 2023-11-09T13:05:38Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+
+    for (Map.Entry<String, String> entry : filterKeyValMap.entrySet()) {
+      String key = entry.getKey();
+      String value = entry.getValue();


filterValue

samdgupi · 2023-11-09T13:07:07Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+      String value = entry.getValue();
+
+      switch (key) {
+        case "type":


We should add a enum for this

OpreationFilterKey{
TYPE
STATUS
}

We convert the key string to the enum. After that we can use switch case

Yes right. Thank you for the correction

samdgupi · 2023-11-09T13:08:33Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    return new OperationRunFilter(operationType, operationStatus);
+  }
+
+  private static Map<String, String> parseFilter(String filter) {


Please add a javadoc explaining the parsing logic

samdgupi · 2023-11-09T13:08:57Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    Map<String, String> filterKeyValMap = new HashMap<>();
+    String[] filterKeyValPairs = filter.split("AND");
+
+    for (String keyValPair : filterKeyValPairs) {


I think you can use regex to parse this easily

Right. Done

samdgupi · 2023-11-09T13:09:38Z

cdap-app-fabric/src/main/java/io/cdap/cdap/internal/operation/OperationLifecycleManager.java

+   *
+   * @param runId run id of the operation to be fetched
+   * @return operation run detail of the operation to be fetched. If not found,
+   *     OperationRunNotFoundException is thrown.S


Also use @throws for documenting exception

samdgupi · 2023-11-09T13:10:39Z

cdap-app-fabric/src/main/java/io/cdap/cdap/internal/operation/OperationRunStore.java

@@ -338,4 +338,4 @@ void clearData() throws IOException {
                    SMALLEST_POSSIBLE_STRING)),
            Range.Bound.INCLUSIVE));
  }
-}
+}


remove this change

samdgupi · 2023-11-09T13:12:00Z

cdap-app-fabric/src/test/java/io/cdap/cdap/internal/operation/SqlOperationRunsStoreTest.java

@@ -69,7 +69,10 @@ protected void configure() {
  }

  @AfterClass
-  public static void afterClass() throws IOException {
+  public static void afterClass(){
+    try{
    pg.close();


you can use Colsables.closequitely

Fix the operation runner test

Add scan operation runs by status method

samdgupi · 2023-11-14T08:57:07Z

cdap-app-fabric/.gitignore

@@ -1,3 +1,4 @@
+.vscode/


Please add in a seperate section similar to intellij files

samdgupi · 2023-11-14T09:23:07Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

@@ -98,4 +176,115 @@ public void failOperation(FullHttpRequest request, HttpResponder responder,
        String.format("Updated status for operation run %s in namespace '%s'.", runId,
            namespaceId));
  }
+
+  private ScanOperationRunsRequest getScanRequest(
+      String namespaceId, String pageToken, Integer pageSize, String filter) {


samdgupi · 2023-11-14T09:25:45Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    return builder.build();
+  }
+
+  private OperationRunFilter getFilter(String filter) {


Please add a TODO for adding unit test for this

samdgupi · 2023-11-14T09:26:37Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    return builder.build();
+  }
+
+  private OperationRunFilter getFilter(String filter) {


Please add this throes IllegalArgumentException

samdgupi · 2023-11-14T09:26:53Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+        case TYPE:
+          try {
+            operationType = OperationType.valueOf(filterValue.toUpperCase());
+          } catch (IllegalArgumentException e) {


No need to catch and rethrow it

samdgupi · 2023-11-14T09:27:53Z

cdap-app-fabric/src/main/java/io/cdap/cdap/gateway/handlers/OperationHttpHandler.java

+    return keyValMap;
+  }
+
+  private void validateNamespace(@Nullable String namespace) throws BadRequestException, NamespaceNotFoundException, AccessException{


I dont think this can be shorthened to

private NamespaceId validateNamespaceId(String namespaceId) throws BadRequestException { try { return new NamespaceId(namespaceId); } catch (IllegalArgumentException e) { throw new BadRequestException(e.getMessage(), e); } }

see SourceControlManagementHttpHandler for reference

…space

Resolved conflicts for rebasing

resolved conflicts while rebasing

…space

Added API handler functions (scan and get) for LRO

fca532a

adrikagupta requested a review from samdgupi November 7, 2023 12:52

adrikagupta changed the title ~~Added API handler functions (scan and get) for LRO~~ [CDAP-20874] API handler functions (scan and get) for LRO Nov 7, 2023

adrikagupta changed the title ~~[CDAP-20874] API handler functions (scan and get) for LRO~~ [CDAP-20874] Added API handler functions (scan and get) for LRO Nov 7, 2023

samdgupi requested changes Nov 7, 2023

View reviewed changes

dli357 and others added 14 commits November 7, 2023 16:19

[CDAP-20879] Use default k8s namespace in hybrid mode for workload id…

57e789e

…entity token exchanges, and do not retry on status code zero for ApiExceptions

Resolved comments in PR

58e0ad8

Resolved comments in PR

594fdfd

Operation runner implementation

27bcbdd

Merge pull request #15409 from cdapio/bugfix/CDAP-20879

cc632b7

[CDAP-20879] Use default k8s namespace in hybrid mode for workload identity token exchanges

do not set WI env vars when NSA is enabled

0f44398

Merge pull request #15413 from cdapio/CDAP-20785

3a7019f

[CDAP-20785] do not set WI env vars when NSA is enabled

Add multi push operation

21bfc91

Merge pull request #15319 from cdapio/operation-api

a606282

CDAP-20809: Operation Runner

Add operation status notification publisher

26c1e1f

Merge pull request #15385 from cdapio/multi-push-operation

4b8c345

Multi push operation

use gcp remote authenticator to fetch token

a497e79

Merge pull request #15415 from cdapio/use-gcp-remote-authenticator

c7e5433

[CDAP-20785] Use GCP remote authenticator to fetch token

Fix the null pointer exception caused by unsafe unboxing of Boolean t…

30e14e2

…o boolean

GnsP reviewed Nov 9, 2023

View reviewed changes

samdgupi and others added 2 commits November 9, 2023 15:42

Merge pull request #15411 from cdapio/operation-state-publisher

43fcc4c

Operation state publisher

Merge pull request #15410 from GnsP/fix-cdap-20877

7544c7a

Fix the null pointer exception caused by unsafe unboxing of Boolean

samdgupi reviewed Nov 9, 2023

View reviewed changes

samdgupi and others added 6 commits November 9, 2023 20:52

Fix the operation runner test

4579b5a

Merge pull request #15420 from cdapio/fix-build-error-op-runner

6863ef4

Fix the operation runner test

Removed .vscode folder

07cdd7b

resolved comments - modified validateNamespace and getFilter

8ee994c

Add scan operation runs by status method

87d79a6

Merge pull request #15424 from cdapio/get-pending-runs

d068939

Add scan operation runs by status method

samdgupi reviewed Nov 14, 2023

View reviewed changes

cdap-app-fabric/.gitignore Outdated

@@ -1,3 +1,4 @@

.vscode/

Copy link

Contributor

samdgupi Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add in a seperate section similar to intellij files

samdgupi requested changes Nov 14, 2023

View reviewed changes

adrikagupta added 9 commits November 15, 2023 07:01

made minor changes, modified key-value pattern, modified validateName…

db43294

…space

Added API handler functions (scan and get) for LRO

b8e73be

Resolved conflicts for rebasing

Resolved comments in PR

e8cd6ca

Resolved conflicts for rebasing

esolved comments in PR

0c5311a

Resolved conflicts for rebasing

Removed .vscode folder

a45a982

resolved comments - modified validateNamespace and getFilter

7eb7600

resolved conflicts while rebasing

made minor changes, modified key-value pattern, modified validateName…

8d50918

…space

resolved an issue that occurred while merging

bef692f

resolved merge conflicts

e53fcaf

adrikagupta changed the title ~~[CDAP-20874] Added API handler functions (scan and get) for LRO~~ [OBSELETE] Added API handler functions (scan and get) for LRO Nov 16, 2023

adrikagupta changed the title ~~[OBSELETE] Added API handler functions (scan and get) for LRO~~ [OBSOLETE] Added API handler functions (scan and get) for LRO Nov 16, 2023

samdgupi closed this May 2, 2024


		@ClassRule public static final TemporaryFolder TEMP_FOLDER = new TemporaryFolder();

		private static EmbeddedPostgres pg;

[OBSOLETE] Added API handler functions (scan and get) for LRO #15408

[OBSOLETE] Added API handler functions (scan and get) for LRO #15408

Conversation

adrikagupta commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment