[CDAP-20993] Namespace and Repository Source control metadata refresh #15641

adrikagupta · 2024-05-07T14:29:50Z

This PR adds the functionality of refreshing namespace and repository source control metadata in one transaction in /repository/apps call before fetching the apps for response. It does the refresh by getting the list of apps from remote repo, cleaning up the repository source control metadata table and then updating the isSynced (sync status) and last modified values accordingly. It also contains fixes to some bugs from the API impl PR in default store.

chtyim · 2024-05-07T15:07:24Z

cdap-app-fabric/src/main/java/io/cdap/cdap/app/store/ListSourceControlMetadataResponse.java

+
+  private final List<SourceControlMetadataRecord> apps;
+  private final String nextPageToken;
+  private final Long lastRefreshTime;


Why use boxed type? Is it nullable? It's better not to use box type if possible. Is there a meaning default value for it that you can use?

Its null if the refresh has not been run yet. I think it would be more appropiate to have default value as null in the API response than a actual long value

I don't think it can ever be null. The value comes from SourceControlMetadataRefresher.getLastRefreshTime, which returns primitive long, and the implementation is defaulting it to 0L

samdgupi · 2024-05-07T15:20:10Z

cdap-common/src/main/java/io/cdap/cdap/common/conf/Constants.java

@@ -2482,6 +2482,10 @@ public static final class SourceControlManagement {
    public static final String REPOSITORY_TTL_SECONDS = "source.control.repository.ttl.seconds";
    public static final String METADATA_MIGRATION_INTERVAL_SECONDS =
        "source.control.metadata.migration.interval.seconds";
+    public static final String METADATA_REFRESH_INTERVAL_SECONDS =


Dont think we need this

samdgupi · 2024-05-07T15:22:48Z

cdap-app-fabric/src/main/java/io/cdap/cdap/app/store/ListSourceControlMetadataResponse.java

+
+  private final List<SourceControlMetadataRecord> apps;
+  private final String nextPageToken;
+  private final Long lastRefreshTime;


Its null if the refresh has not been run yet. I think it would be more appropiate to have default value as null in the API response than a actual long value

...app-fabric/src/main/java/io/cdap/cdap/internal/app/services/ApplicationLifecycleService.java

samdgupi · 2024-05-07T15:24:59Z

...app-fabric/src/main/java/io/cdap/cdap/internal/app/services/ApplicationLifecycleService.java

@@ -415,7 +427,9 @@ public ApplicationDetail getLatestAppDetail(ApplicationReference appRef)
   *                                                reference is not found.
   */
  public SourceControlMetadataRecord getSourceControlMetadataRecord(ApplicationReference appRef)
-      throws SourceControlMetadataNotFoundException {
+      throws SourceControlMetadataNotFoundException, RepositoryNotFoundException {
+    // Checking if repository config exists for the namespace


IIRC RepositoryNotFoundException would be interpreted as 404 right

samdgupi · 2024-05-07T15:25:14Z

...rc/main/java/io/cdap/cdap/internal/app/services/NamespaceSourceControlMetadataRefresher.java

+
+/**
+ * Service for refreshing namespace and repository source control metadata of a namespace at
+ * scheduled intervals and manually.


Change comment

...rc/main/java/io/cdap/cdap/internal/app/services/NamespaceSourceControlMetadataRefresher.java

samdgupi · 2024-05-07T15:27:42Z

...rc/main/java/io/cdap/cdap/internal/app/services/NamespaceSourceControlMetadataRefresher.java

+    return this.lastRefreshTime.get();
+  }
+
+  private void updateSourceControlMetaWithNoTransaction(ApplicationReference appRef,


name it updateSourceControlMeta

samdgupi · 2024-05-07T15:30:18Z

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

-          SourceControlMetadataRecord record = lastRecord.get();
-          return !pageLimitReached || record == null ? null : record.getName();
-        });
+    List<SourceControlMetadataRecord> apps = new ArrayList<>();


we should check if repoconfig is present here

...p-fabric/src/main/java/io/cdap/cdap/gateway/handlers/SourceControlManagementHttpHandler.java

samdgupi · 2024-05-07T19:11:21Z

...rc/main/java/io/cdap/cdap/internal/app/services/NamespaceSourceControlMetadataRefresher.java

+      return;
+    }
+
+    if (namespaceSourceControlMeta.getLastSyncedAt() != null


add few comments explaining each section

samdgupi · 2024-05-07T19:13:08Z

...rc/main/java/io/cdap/cdap/internal/app/services/NamespaceSourceControlMetadataRefresher.java

+    List<SourceControlMetadataRecord> records = new ArrayList<>();
+    String type = EntityType.APPLICATION.toString();
+
+    store.scan(request, type, records::add);


you can do

if (!repoFiles.contains(record.getName())) { store.delete(new ApplicationReference(record.getNamespace(), record.getName())); }

inside scan

samdgupi

Please update the

chtyim · 2024-05-07T19:29:18Z

cdap-app-fabric/src/main/java/io/cdap/cdap/app/store/ListSourceControlMetadataResponse.java

+    return nextPageToken;
+  }
+
+  public Long getLastRefreshTime() {


Annotate with @Nullable

chtyim · 2024-05-07T19:29:47Z

cdap-app-fabric/src/main/java/io/cdap/cdap/app/store/ListSourceControlMetadataResponse.java

+   * @param lastRefreshTime The timestamp of the last refresh operation.
+   */
+  public ListSourceControlMetadataResponse(List<SourceControlMetadataRecord> apps,
+      String nextPageToken, Long lastRefreshTime) {


Annotate parameters that can be null with @Nullable.

chtyim · 2024-05-07T19:30:15Z

cdap-app-fabric/src/main/java/io/cdap/cdap/app/store/SingleSourceControlMetadataResponse.java

+public class SingleSourceControlMetadataResponse {
+
+  private final SourceControlMetadataRecord app;
+  private final Long lastRefreshTime;


Is this nullable? If so, please annotate accordingly.

chtyim · 2024-05-07T19:31:35Z

cdap-app-fabric/src/main/java/io/cdap/cdap/app/store/ListSourceControlMetadataResponse.java

+    return apps;
+  }
+
+  public String getNextPageToken() {


Are these getter methods used? If not, no need to define them.

chtyim · 2024-05-07T19:36:13Z

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

+    boolean pageLimitReached;
+    pageLimitReached = applicationLifecycleService.scanSourceControlMetadata(
+        scanRequest, batchSize,
+        apps::add);


In the old code we don't buffer the app, but in the new code we do. I remember do removed the buffered to avoid high memory consumption, but now we are bringing the same problem back. Is buffering needed? Seems like we can refactor the JsonPaginatedListResponder so that it can emit extra fields (lastRefreshTime in this case) before closing the json object.

We can extend JsonPaginatedListResponder to also add additional field.
@chtyim Is it ok to take this up as a fast followup for the change ?

chtyim · 2024-05-07T19:37:36Z

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

+    SourceControlMetadataRecord record = apps.isEmpty() ? null :apps.get(apps.size() - 1);
+    String nextPageToken = !pageLimitReached || record == null ? null :
+        record.getName();
+    long lastRefreshTime = applicationLifecycleService.getLastRefreshTime(namespaceId);


Seems like unwise to sync all apps, shouldn't we just sync the apps fetched in this page?

As we are cloning the full repository which would be time taking it might be wasteful to just update few pipelines.

chtyim · 2024-05-07T19:39:20Z

...ic/src/main/java/io/cdap/cdap/internal/app/sourcecontrol/SourceControlMetadataRefresher.java

+ * NamespaceSourceControlMetadataRefreshService scheduler instances.
+ */
+public class SourceControlMetadataRefresher {
+  private ConcurrentMap<NamespaceId, Long>


chtyim · 2024-05-07T19:39:30Z

...ic/src/main/java/io/cdap/cdap/internal/app/sourcecontrol/SourceControlMetadataRefresher.java

+      lastRefreshTimes = new ConcurrentHashMap<>();
+  private final TransactionRunner transactionRunner;
+  private final SourceControlOperationRunner sourceControlOperationRunner;
+  private final SecureStore secureStore;


unused field

chtyim · 2024-05-07T19:39:43Z

...ic/src/main/java/io/cdap/cdap/internal/app/sourcecontrol/SourceControlMetadataRefresher.java

+  private final TransactionRunner transactionRunner;
+  private final SourceControlOperationRunner sourceControlOperationRunner;
+  private final SecureStore secureStore;
+  private static final Logger LOG = LoggerFactory.getLogger(


unsued. Also, static field should comes before non static fields

chtyim · 2024-05-07T19:40:30Z

...ic/src/main/java/io/cdap/cdap/internal/app/sourcecontrol/SourceControlMetadataRefresher.java

+   * Constructs a SourceControlMetadataRefreshService instance.
+   */
+  @Inject
+  public SourceControlMetadataRefresher(TransactionRunner transactionRunner,


injected constructor shouldn't be public. Use package private (i.e. no access modifier).

chtyim · 2024-05-07T19:42:56Z

...ic/src/main/java/io/cdap/cdap/internal/app/sourcecontrol/SourceControlMetadataRefresher.java

+   * @return The timestamp of the last refresh
+   */
+  public long getLastRefreshTime(NamespaceId namespaceId) {
+    return lastRefreshTimes.getOrDefault(namespaceId, 0L);


So the refresh time is only kept in memory? Meaning the information will be gone if the pod restarted?

Yes we do not have a good place to store this information.

chtyim · 2024-05-07T19:44:57Z

...app-fabric/src/main/java/io/cdap/cdap/internal/app/services/ApplicationLifecycleService.java

+      throws IOException, RepositoryNotFoundException {
+    NamespaceId namespaceId = new NamespaceId(request.getNamespace());
+    // Checking if repository config is present in the namespace
+    sourceControlMetadataRefresher.getRepositoryMeta(namespaceId);


What's the point of calling get without using the result?

We want to check if RepositoryConfig exist or throw execption otherwise. @adrikagupta Please correct me if I am wrong but you chose this function to reduce duplicated code to fetch namespace repository config

chtyim · 2024-05-07T19:51:42Z

...ic/src/main/java/io/cdap/cdap/internal/app/sourcecontrol/SourceControlMetadataRefresher.java

+    lastRefreshTimes.putIfAbsent(namespaceId, createRefreshService(namespaceId).refreshMetadata());
+  }
+
+  public RepositoryMeta getRepositoryMeta(NamespaceId namespace)


This method seems duplicating the one in SourceControlManagementService. Why need to duplicate?

This class is actually a member of SourceControlManagementService. We should move this to a common place. @adrikagupta I think you added a TODO for the same ?

chtyim · 2024-05-07T19:57:25Z

...bric/src/main/java/io/cdap/cdap/internal/app/store/RepositorySourceControlMetadataStore.java

      Long lastSynced) throws IOException {
    StructuredTable repoTable = getRepositorySourceControlMetadataTable();
    repoTable.upsert(getAllFields(appRef, isSynced, lastSynced));
  }

+  /**


I can't comment on line 120. But there is 21 lines of duplicated code detected by IntelliJ with the NamespaceSourceControlMetadataStore class. Why are we duplicating code?

This two classes are handilng crud functions for two different (but similar) tables. Hence the duplication.

added implementation of peridic refresh for source control metadata

aedcb1f

chtyim reviewed May 7, 2024

View reviewed changes

samdgupi reviewed May 7, 2024

View reviewed changes

adrikagupta force-pushed the sync-status-last branch from c7a05e4 to 06dc4b3 Compare May 7, 2024 17:20

adrikagupta changed the title ~~[CDAP-20993]~~ [CDAP-20993] Namespace and Repository Source control metadata refresh May 7, 2024

updating namespace and repository source control metadata manually

5b6f2dd

adrikagupta force-pushed the sync-status-last branch from 06dc4b3 to 5b6f2dd Compare May 7, 2024 17:40

samdgupi reviewed May 7, 2024

View reviewed changes

samdgupi added the build Triggers github actions build label May 7, 2024

samdgupi approved these changes May 7, 2024

View reviewed changes

chtyim reviewed May 7, 2024

View reviewed changes

[CDAP-20993] Namespace and Repository Source control metadata refresh #15641

Are you sure you want to change the base?

[CDAP-20993] Namespace and Repository Source control metadata refresh #15641

Conversation

adrikagupta commented May 7, 2024 • edited

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

samdgupi left a 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

samdgupi May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdgupi May 7, 2024 • edited

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

adrikagupta commented May 7, 2024 •

edited

samdgupi May 7, 2024 •

edited

samdgupi May 7, 2024 •

edited