Many ToDos and Deprecations in the code - Needs Cleanup & Refactoring

[sravankorumilli]

@arinda-arif, @irainia can one of you analyse these and come up with action items thanks.

[irainia]

The following is a general analysis of the currently active todos and some deprecation of the code. The base of this analysis is the state up to this commit.

Note: This is not the final analysis, further updates will be written later.

package/file	line	description	action item
cmd/	-	on a general cmd package structure, we can try further refactor it. for further detail, we can try to check this issue	refactor
cmd/	-	in some files under this package, we have method loadConfig, which are similar. we need to check whether it's possible to extract them to avoid duplication.	analysis
api/handler/v1beta1/adapter.go	268	function ToProjectProtoWithSecrets is unused	remove
cmd/job/render.go	68	we need further discussion on this	discussion
cmd/server/optimus.go	140	we need further discussion on this	discussion
cmd/server/optimus.go	198	if an error log is required, then we might need to update the clean-up implementation to also return the error. if we don't return an error, then we need to send a logger to the clean-up function somehow. the latter can be implemented without issue as long as the initializing clean-up function does require a log other than for clean-up. if it's just for clean-up, then better send an error instead.	refactor
cmd/server/optimus.go	237	we should remove the global variable. if possible, we can put it inside the package where it's being used	discussion
cmd/server/optimus.go	381	we can break down job service into smaller chunks according to its responsibilities, such as replay. this will resolve the todo related to replay.	refactor
cmd/server/scheduler.go	64	we can add logging for this todo	refactor
config/loader.go	33	we can remove init function	remove
ext/scheduler/airflow2/airflow.go	138	both DeployJobs and DeployJobsVerbose are still used, we need to decide on how to remove one of them as mentioned. this could be done only if the create job specification uses the newer flow	remove
ext/scheduler/airflow2/airflow.go	218	method ListJobs can get record from db	refactor
ext/scheduler/airflow2/client.go	63	todo that mentions about removing project spec, need further discussion as to why it's being mentioned as such	discussion
store/	-	refactor store to separate project, namespace, job, and resource. break each of them into smaller implementations or structures for easier to maintain. any duplication should be avoided. move them into a separate place. on this, we can come to a convention on how to design the components. but, the main point is, there's a separation between one store and another to avoid confusion. relationship between one store and another should be avoided as much as possible. the implementations depend highly on how we design our store. such as: *) all repository are put within store package; or *) stores are put together with service, and other components, making them completely independent of each other; or other better implementations	discussion

[irainia]

The following is the continuation of the above comment. Others will be added later.

package/file	line	description	action item
job/dependency_resolver.go	35	we can deprecate projectJobSpecRepoFactory field if we deprecate the Resolve method. this can happen only if job create uses the newer flow. as a side note, we can also try to rename this resolver into getter, for example. because, in the nature, what it does is getting things. of course, if we want to resolve (meaning we try to update the existing request), we can do so as well. another, if all job flows are migrated to the newer one, we can just resolves all the mentioned todos. this also has impacts on interface DependencyResolver	remove
job/deploy_manager.go	126	minimize query call to get deployment request. one approach we can consider is to query all from database, then iterate to spawn a deploy worker for each	refactor
job/service.go	313, 337	we can include job downstream when deleting, since we want to prevent doing so if there's any. one way to do that is by mentioning all of the downstream jobs, but it can be bloated. we can come up with another solution, for example there's a mechanism to have a summary of the jobs or showing only some samples of it.	refactor
job/service.go	859	we can try to explore options to get the secret properly during identifying dependencies from plugin. note, we can also try to rename the plugin method for clearer message as to what it returns. for example, GenerateDependencyURNs to express that this method gets a resource URN, not a job name	refactor
models/job.go	417	interface JobService does a lot of things. ideally, interface should be as small as possible. however, sometime we can't really split a contract if they are within the same scope. in relation to this, JobService does more things than the one for Job only. for example, it handles a job run creation, another, it handles refresh. apart from that, there are some that could be included in the others. for example, we have GetByFilter and GetByName. so for this, it will be ideal if we can make the interface as thin as possible by extracting those that are within a separate scope.	refactor
plugin/v1beta1/base/client.go	108	method MakeFatalOnConnErr need further discussion, as this might be impacted by the currently on going pr	discussion
service/errors.go	56	we can refactor this code to include custom message to the overall message. the approach can be varies, such as we store in the field first, or directly appending it to the message	refactor
store/local/job_spec_adapter.go	468	we should avoid possibilities of panic as much as possible. for example, jobspecconfigitem, when we adapting it, panic could happen.	refactor
store/postgres/secret_repository.go	-	we need to discuss about the action to be done when secret is empty. currently, the implementation is being put under Secret structure. it depends on the implementation, but we this implementation to encrypt and decrypt can be put into its own service	discussion

[irainia]

The following is the continuation of the above comment. Others will be added later.

package/file	line	description	action item
-	-	CreateJobSpecification still uses the old deployment flow: *) duplication on job/Sync and job/Deploy; *) duplication on airflow/DeployJobs and airflow/DeployJobsVerbose; *)unnecessary and complex job/GetDependencyResolvedSpecs; *) duplication on job/Resolve and ResolveStaticDependencies; *) unnecessary job/resolveInferredDependencies, job/resolveStaticDependencies, job/extractDependency	refactor
job/service.go	696	populateDownstreamDAGs' in job/service.go is too tied up with Airflow scheduler. The logic itself is quite similar with 'buildMultiRootDependencyTree' in job/priority_resolver.go	refactor
job/service.go	532	PrepareJobSpecMap is being used by Replay, ReplayDryRun, and Backup to get the lineage of the requested job. This is not necessary, too complex, and inefficient as job sources have been persisted.	discussion

[arinda-arif]

Note based on discussion with @irainia @sravankorumilli @sbchaos:

No	Description	Note
1	[cmd/] on a general cmd package structure, we can try further refactor it. for further detail, we can try to check this issue	agreed
2	[cmd/] in some files under this package, we have method loadConfig, which are similar. we need to check whether it's possible to extract them to avoid duplication.	remove this, making inline function ToProjectProtoWithSecrets is unused
3	[cmd/job/render.go] we need further discussion on this	will remove render command including compiler.DumpAssets
4	[cmd/server/optimus.go] we need further discussion on this	should just pass conf.serve.DB
5	[cmd/server/optimus.go] if an error log is required, then we might need to update the clean-up implementation to also return the error. if we don't return an error, then we need to send a logger to the clean-up function somehow. the latter can be implemented without issue as long as the initializing clean-up function does require a log other than for clean-up. if it's just for clean-up, then better send an error instead.	add error logging inside cleanup function
6	[cmd/server/optimus.go] we should remove the global variable. if possible, we can put it inside the package where it's being used	agreed
7	[cmd/server/optimus.go] we can break down job service into smaller chunks according to its responsibilities, such as replay. this will resolve the todo related to replay.	agreed
8	[cmd/server/scheduler.go] we can add logging for this todo in cmd/server/scheduler.go line 64	agreed
9	[config/loader.go] we can remove init function in config/loader.go	agreed
10	[ext/scheduler/airflow2/airflow.go] both DeployJobs and DeployJobsVerbose are still used, we need to decide on how to remove one of them as mentioned. this could be done only if the create job specification uses the newer flow	remove DeployJobs and rename DeployJobsVerbose
11	[ext/scheduler/airflow2/airflow.go] method ListJobs can get record from db	agreed
12	[ext/scheduler/airflow2/client.go] todo that mentions about removing project spec in airflow2/client.go	we can pass only what's needed
13	[store/] refactor store to separate project, namespace, job, and resource. break each of them into smaller implementations or structures for easier to maintain. any duplication should be avoided. move them into a separate place. on this, we can come to a convention on how to design the components. but, the main point is, there's a separation between one store and another to avoid confusion. relationship between one store and another should be avoided as much as possible. the implementations depend highly on how we design our store. such as: *) all repository are put within store package; or *) stores are put together with service, and other components, making them completely independent of each other; or other better implementations	we need to identify what kind of access pattern we should follow.
14	[job/dependency_resolver.go] we can deprecate projectJobSpecRepoFactory field if we deprecate the Resolve method. this can happen only if job create uses the newer flow. as a side note, we can also try to rename this resolver into getter, for example. because, in the nature, what it does is getting things. of course, if we want to resolve (meaning we try to update the existing request), we can do so as well. another, if all job flows are migrated to the newer one, we can just resolves all the mentioned todos. this also has impacts on interface DependencyResolver	agreed
15	[job/deploy_manager.go] minimize query call to get deployment request. one approach we can consider is to query all from database, then iterate to spawn a deploy worker for each	agreed but minor
16	[job/service.go] we can include job downstream when deleting, since we want to prevent doing so if there's any. one way to do that is by mentioning all of the downstream jobs, but it can be bloated. we can come up with another solution, for example there's a mechanism to have a summary of the jobs or showing only some samples of it.	agreed
17	[job/service.go] we can try to explore options to get the secret properly during identifying dependencies from plugin. note, we can also try to rename the plugin method for clearer message as to what it returns. for example, GenerateDependencyURNs to express that this method gets a resource URN, not a job name	ProjectSpec no longer need to be passed. For renaming, we can consider GenerateUpstreamResourceURNs
18	[models/job.go] interface JobService does a lot of things. ideally, interface should be as small as possible. however, sometime we can't really split a contract if they are within the same scope. in relation to this, JobService does more things than the one for Job only. for example, it handles a job run creation, another, it handles refresh. apart from that, there are some that could be included in the others. for example, we have GetByFilter and GetByName. so for this, it will be ideal if we can make the interface as thin as possible by extracting those that are within a separate scope.	agreed
19	[plugin/v1beta1/base/client.go] method MakeFatalOnConnErr need further discussion, as this might be impacted by the currently on going pr	need to return error instead of panic
20	[service/errors.go] we can refactor this code to include custom message to the overall message. the approach can be varies, such as we store in the field first, or directly appending it to the message	we can append to existing message
21	[store/local/job_spec_adapter.go] we should avoid possibilities of panic as much as possible. for example, jobspecconfigitem, when we adapting it, panic could happen.	already fixed in main branch
22	[store/postgres/secret_repository.go] we need to discuss about the action to be done when secret is empty. currently, the implementation is being put under Secret structure. it depends on the implementation, but we this implementation to encrypt and decrypt can be put into its own service	already fixed (moved to service layer)
23	[-] CreateJobSpecification still uses the old deployment flow: *) duplication on job/Sync and job/Deploy; *) duplication on airflow/DeployJobs and airflow/DeployJobsVerbose; *)unnecessary and complex job/GetDependencyResolvedSpecs; *) duplication on job/Resolve and ResolveStaticDependencies; *) unnecessary job/resolveInferredDependencies, job/resolveStaticDependencies, job/extractDependency	need to refactor CreateJobSpecification
24	[job/service.go] populateDownstreamDAGs' in job/service.go is too tied up with Airflow scheduler. The logic itself is quite similar to 'buildMultiRootDependencyTree' in job/priority_resolver.go	need to remove the duplication
25.	[job/service.go] PrepareJobSpecMap is being used by Replay, ReplayDryRun, and Backup to get the lineage of the requested job. This is not necessary, too complex, and inefficient as job sources have persisted.	need to refactor Replay, ReplayDryRun & Backup