delete this

done

just expose a function in applicationLifecycleService to get the last refresh time

check if there is some Optional return

you should always update repostore

same as above

nit: Please add temporary variables to hold isSync, LastModified value. This way it would be easy to understand what is being passed to write()

this should always happen before the if

But addRefreshService should be called only if it is not already present in refreshSchedulers, right?

addRefreshService should have putIfAbsent that would take care of existing scheduler issue

debug log. and add it after you added the scheduler

use putIfNotPresent. The current logic is not threadsafe

stopandwait()

you can do

  if (getRepositoryMeta(namespaceId) != null) {
        addRefreshService(namespaceId);
      }

here only. no need for a list

Please do not do this. Otherwise in case if the flag is false the API would not work at all. Just skip refresh if the flag is not enabled

dont name it record. rather name it app. Similar to how list api is calling this apps

Please call runrefreshservice here

Please fix. Set it to 10 min by default

Please fix checkstyle

Please fix checkstyle

Should we not also write when meta.getSourceControlMeta() != null ?

Should we not also write when meta.getSourceControlMeta() != null ?

Sure, I have changed to -

if (meta.getSourceControlMeta() != null) {
          Instant lastSyncedAt = meta.getSourceControlMeta().getLastSyncedAt();
          repoStore.write(appRef, true, lastSyncedAt == null ? null :
              lastSyncedAt.toEpochMilli());
        }
        else{
          ImmutablePair<Long, Boolean> lastSyncedAndStatusPair = repoStore.get(appRef);
          if(lastSyncedAndStatusPair != null) {
            repoStore.write(appRef, false, lastSyncedAndStatusPair.getFirst());
          }
        }

Three cases should be considered:

1. If we are deploying an app in namespace and hasn't been pulled or pushed before - In this case we check, scm meta is null then we dont write anything in repo table since it is not present in the repo table.
2. If we are deploying an app already been pulled/pushed before - In this case the record will be present in repo table hence we will change the sync status but retain the last modified.
3. If we are pulling and deploying - In this case since scm meta will not be null we will be setting the values in repo table.

I feel the previous implementation might have also worked based on the concept that if we are pulling and deploying or are deploying an app which has been pulled/pushed before, the record would be there in the repo table.

add the service only if flag is enabled

But it is possible that the user disables the periodic refresh flag but keeps manual refresh flag enabled

why are we triggering here again ?

 public void addRefreshService(NamespaceId namespaceId) {
    NamespaceSourceControlMetadataRefreshService service =
        refreshSchedulers.putIfAbsent(namespaceId, createRefreshService(namespaceId));
    if (service == null || isSourceControlMetadataPeriodicRefreshFlagEnabled()) {
      refreshSchedulers.get(namespaceId).start();
    } else {
      refreshSchedulers.get(namespaceId).triggerManualRefresh(true);
    }
  }

If we are setting the repo config for the first time, it creates refresh service, calls start function, and performs runOneIteration.
When we are updating the repo config, it is already present in refreshSchedulers. If start() function is called, it will see that it has already started and will not call runOneIteration instantly. So we have to trigger it manually so that the changes are reflected in the git pipelines page.

remove this as we already have a feature flag

why is this changed needed ? why we cant use SourceControlMetadataRecord

Why cant we just use SourceControlMetadataRecord ?

As mentioned before please add a separate function to toggle sync status only

This is not correct as refreshSchedulers.get(namespaceId) can be null. Rather get refreshSchedulers.get(namespaceId) in a temp variable and check if it is null

nit: Use forEach

Please throw exception and let the http exception handler to handle the response. If you send here, you need to return from the catch block, otherwise the flow will continue.

The lastRecord variable seems unnecessary. The same can be achieved by apps.isEmpty() and apps.get(apps.size() - 1).

This would execute another SQL in another transaction, which may be inconsistent with what has been returned by the scanSourceControlMetadata call. I think it's better for the scan method to return a response that contains apps, page limit, and last scan time. This makes sure all the data are coming from the same transaction.

Same as above. Please throw. The old code was wrong too, please don't follow with that pattern.

Should be using primitive long instead.

Why the refreshment is enabled only when there is a scan?

So we have one thread per namespace for the refresh? Seems not an efficient way of using thread. Ideally we only need a small scheduled thread pool for all namespaces refresh?

You shouldn't need an atomic boolean for this. The AbstractScheduledService should have the correct guarantee of how the scheduled run is.

Use primitive long.

Everything that happens in one run should happens in the same transaction. Using admin will create a different transaction boundary.

Use interface Set as the variable type.

Use placeholder {} for logging instead of string concat. Also, seems like a warning than error.

Use placeholder {} instead of string concat for loggin.

Should use primitive long.

Use List as the type.

What does the store.delete actually do? Asking it because this method is nested inside the transactional scan method.

Should use primitive long.