[CDAP-21096] Split Appfabric into stateless service and stateful processor #15773

vsethi09 · 2024-12-16T18:44:35Z

Context

Make appfabric service stateless by splitting it into appfabric service to server HTTP requests and appfabric processor to run subscriber services and process message.

Change Description

Added new main class AppFabricProcessorServiceMain for appfabric processor.
Added AppFabricProcessorService which is used by AppFabricProcessorServiceMain.
Removed messaging subscriber services from AppFabricServer and moved them to AppFabricProcessorService.
HTTP handlers are hosted only in AppFabricServer and AppFabricProcessorService does not have any HTTP handlers.
Updated test classes to run Appfabric Processor along with Appfabric Server wherever needed.

Verification

Unit Tests:
- Updated test classes to run Appfabric Processor along with Appfabric Server wherever needed.
- Added UT for AppFabricProcessorServiceTest to test Start and Stop (similar to AppFabricServerTest).
- Note: UT cannot be added for AppFabricProcessorServiceMain as it does not host and HTTP handlers and runs only subscriber services.
CDAP sandbox: Tested basic flows.
Deployed locally built image to a k8s cluster and verified by deployed CRD and CR. ([CDAP-21096] Split Appfabric into stateless service and stateful processor cdap-operator#123)

itsankit-google · 2024-12-19T18:25:27Z

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

+      CredentialProviderService credentialProviderService,
+      NamespaceCredentialProviderService namespaceCredentialProviderService,


why do we need CredentialProviderService in processor?

How will it work in this case if the processor service does not host any handlers but GcpWorkloadIdentityHttpHandler runs in AppfabricService?

Not needed. Removed.

itsankit-google · 2024-12-19T18:25:53Z

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

@@ -0,0 +1,216 @@
+/*
+ * Copyright © 2014-2020 Cask Data, Inc.


nit: 2024

itsankit-google · 2025-01-02T16:52:35Z

...p-fabric/src/test/java/io/cdap/cdap/internal/app/services/AppFabricProcessorServiceTest.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright © 2014-2018 Cask Data, Inc.


nit: 2025

tivv · 2025-01-02T22:24:32Z

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

@@ -0,0 +1,195 @@
+/*
+ * Copyright © 2014-2024 Cask Data, Inc.


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

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

tivv · 2025-01-07T23:59:10Z

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

+      NamespaceCredentialProviderService namespaceCredentialProviderService,
+      ProvisioningService provisioningService,
+      BootstrapService bootstrapService,
+      SystemAppManagementService systemAppManagementService,


SystemAppManagementService#bootStrapSystemAppConfigDir is (a) slow (b) I am not sure what happens if we try to do it in parallel. Are there harm on keeping ApplicationLifecycleService/ProgramLifecycleService in the Processor as well?

tivv · 2025-01-08T00:00:53Z

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

-  private final ProgramStopSubscriberService programStopSubscriberService;
-  private final RunRecordCorrectorService runRecordCorrectorService;
-  private final RunDataTimeToLiveService runDataTimeToLiveService;
-  private final ProgramRunStatusMonitorService programRunStatusMonitorService;
  private final RunRecordMonitorService runRecordCounterService;


This guy is not designed to run in multiple instances. It has internal tracker for "number of concurrent starts". Our counters will be wrong if we start it everywhere, e.g. removeRequest is mostly called by ProgramNotificationSubscriberServicem while new requests are added by the handler. We need to make this mechanism distributed. @masoud-io FYI

As discussed, in this PR I have moved the HTTP handlers for program, app, workflow to processors.

They can be moved back to the Appfabric server after the issue due to in-memory cache is fixed. Created Jira for follow up PR: CDAP-21112 Move HTTP handler from Appfabric processor to server after fixing ProgramRuntimeService and RunRecordMonitorService

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

tivv · 2025-01-09T20:04:17Z

...-master/src/main/java/io/cdap/cdap/master/environment/k8s/AppFabricProcessorServiceMain.java

+        new AuditModule(),
+        new AuthorizationModule(),
+        new AuthorizationEnforcementModule().getMasterModule(),
+        Modules.override(new AppFabricServiceRuntimeModule(cConf).getDistributedModules())


Are you using same runtime module for Service and Processor? How does it decide what to bring up where?

Yes, same module is used.

AppFabricServer and AppFabricProcessorService decide what to bring up for their service.

I feel it's a bit too easy to mess up. We define both things and then if anyone refers to a thing with a different name, we get all the handlers up... It's probably not the biggest deal as Processor handlers won't be able to come up without necessary services, but it also introduces a hidden assumption.
May I ask you to add some explicit thing, e.g. add a parameter to AppFabricServiceRuntimeModule constructor (PROCESSOR_HANDLERS/SERVER_HANDLERS/BOTH_HANDLERS) and bind only the handlers this module is constructed for? This would make it explicit and ensures it's hard to mess it up.

Since this will touch multiple files. I'm addressing this comment in a separate PR: #15799

tivv

LGTM with one comment to address.

tivv · 2025-01-09T21:45:49Z

...-master/src/main/java/io/cdap/cdap/master/environment/k8s/AppFabricProcessorServiceMain.java

+        new AuditModule(),
+        new AuthorizationModule(),
+        new AuthorizationEnforcementModule().getMasterModule(),
+        Modules.override(new AppFabricServiceRuntimeModule(cConf).getDistributedModules())


I feel it's a bit too easy to mess up. We define both things and then if anyone refers to a thing with a different name, we get all the handlers up... It's probably not the biggest deal as Processor handlers won't be able to come up without necessary services, but it also introduces a hidden assumption.
May I ask you to add some explicit thing, e.g. add a parameter to AppFabricServiceRuntimeModule constructor (PROCESSOR_HANDLERS/SERVER_HANDLERS/BOTH_HANDLERS) and bind only the handlers this module is constructed for? This would make it explicit and ensures it's hard to mess it up.

…essor Fix RunRecordCorrectorService and AuditLogSubscriberService Fix RunRecordCorrectorService and AuditLogSubscriberService Add cdap system service for appfabric processor Move AppLifecycle, ProgramLifecycle and Workflow HTTP handlers to appfabric server Fix unit tests

sonarqubecloud · 2025-01-12T15:08:21Z

Quality Gate failed

Failed conditions
16.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

tivv

LGTM

tivv · 2025-01-12T18:50:21Z

...-master/src/main/java/io/cdap/cdap/master/environment/k8s/AppFabricProcessorServiceMain.java

+        new AuditModule(),
+        new AuthorizationModule(),
+        new AuthorizationEnforcementModule().getMasterModule(),
+        Modules.override(new AppFabricServiceRuntimeModule(cConf).getDistributedModules())


vsethi09 added the build Triggers github actions build label Dec 16, 2024

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch 7 times, most recently from 6874823 to 75ec19d Compare December 19, 2024 09:43

vsethi09 marked this pull request as ready for review December 19, 2024 09:43

vsethi09 requested review from tivv, chtyim, masoud-io, sahusanket and itsankit-google December 19, 2024 09:44

vsethi09 changed the title ~~[WIP][CDAP-21096] Split Appfabric into stateless service and stateful processor~~ [CDAP-21096] Split Appfabric into stateless service and stateful processor Dec 19, 2024

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch from 75ec19d to b3e3e17 Compare December 19, 2024 10:11

itsankit-google reviewed Dec 19, 2024

View reviewed changes

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch 2 times, most recently from f88bc89 to ce8bda2 Compare December 23, 2024 11:06

vsethi09 requested a review from itsankit-google December 30, 2024 07:38

itsankit-google reviewed Jan 2, 2025

View reviewed changes

tivv reviewed Jan 2, 2025

View reviewed changes

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch 6 times, most recently from 227670b to 510685e Compare January 6, 2025 17:28

vsethi09 requested a review from tivv January 6, 2025 17:31

tivv reviewed Jan 8, 2025

View reviewed changes

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch 13 times, most recently from aed827e to e3a85e1 Compare January 9, 2025 18:56

vsethi09 requested a review from tivv January 9, 2025 19:26

tivv reviewed Jan 9, 2025

View reviewed changes

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch 2 times, most recently from 6af62c9 to eca1abb Compare January 9, 2025 20:45

tivv approved these changes Jan 9, 2025

View reviewed changes

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch 3 times, most recently from bae34bc to 0a2c5b8 Compare January 12, 2025 08:26

vsethi09 force-pushed the feature/CDAP-21096_split_appfabric_service_processor branch from 0a2c5b8 to 7a71a52 Compare January 12, 2025 13:24

vsethi09 merged commit 94d7671 into develop Jan 12, 2025
11 of 12 checks passed

vsethi09 deleted the feature/CDAP-21096_split_appfabric_service_processor branch January 12, 2025 15:16

vsethi09 mentioned this pull request Jan 12, 2025

[CDAP-21096] Parametrize the AppFabricServiceRuntimeModule #15799

Merged

3 tasks

tivv reviewed Jan 12, 2025

View reviewed changes

vsethi09 mentioned this pull request Jan 15, 2025

[CDAP-21096] Remove in-memory launching queue from RunRecordMonitorService #15800

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDAP-21096] Split Appfabric into stateless service and stateful processor #15773

[CDAP-21096] Split Appfabric into stateless service and stateful processor #15773

vsethi09 commented Dec 16, 2024 •

edited

Loading

itsankit-google Dec 19, 2024 •

edited

Loading

itsankit-google Dec 19, 2024

vsethi09 Dec 23, 2024

itsankit-google Dec 19, 2024

vsethi09 Dec 23, 2024

itsankit-google Jan 2, 2025

vsethi09 Jan 6, 2025

tivv Jan 2, 2025

vsethi09 Jan 6, 2025

tivv Jan 7, 2025

tivv Jan 8, 2025

vsethi09 Jan 9, 2025

tivv Jan 9, 2025

vsethi09 Jan 9, 2025

tivv Jan 9, 2025

vsethi09 Jan 12, 2025

tivv Jan 12, 2025

tivv left a comment

tivv Jan 9, 2025

sonarqubecloud bot commented Jan 12, 2025

tivv left a comment

tivv Jan 12, 2025

		CredentialProviderService credentialProviderService,
		NamespaceCredentialProviderService namespaceCredentialProviderService,

[CDAP-21096] Split Appfabric into stateless service and stateful processor #15773

[CDAP-21096] Split Appfabric into stateless service and stateful processor #15773

Conversation

vsethi09 commented Dec 16, 2024 • edited Loading

Context

Change Description

Verification

itsankit-google Dec 19, 2024 • edited Loading

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

tivv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 12, 2025

Quality Gate failed

tivv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsethi09 commented Dec 16, 2024 •

edited

Loading

itsankit-google Dec 19, 2024 •

edited

Loading