package: Add `--exclude` option to exclude components from being started. #492

junhaoliao · 2024-07-23T20:43:32Z

Description

When developing / testing certain components which provide a live reload debug server, other CLP components also need to be started for functionality verifications. Previously, developers have been starting the whole CLP package and later stopping the component(s) to be debugged, to avoid resource (e.g. TCP host + port) conflicts. The approach works but has proven to be inconvenience and prone to manual errors. This PR:

Add --exclude option to exclude components from being started.

Validation performed

Built the package.
```
cd <clp-root>
task
```

Started the package, excluding components webui and log_viewer_webui.

cd build/clp-package/sbin
./start-clp.sh --exclude webui log_viewer_webui

Observed all components started except webui and log_viewer_webui.

…ted.

junhaoliao · 2024-07-23T20:45:52Z

components/clp-package-utils/clp_package_utils/scripts/start_clp.py

+        if target in (ALL_TARGET_NAME, DB_COMPONENT_NAME) and DB_COMPONENT_NAME not in exclude:
            start_db(instance_id, clp_config, conf_dir)
-        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, DB_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, DB_COMPONENT_NAME) and CONTROLLER_TARGET_NAME not in exclude and DB_COMPONENT_NAME not in exclude:
            create_db_tables(instance_id, clp_config, container_clp_config, mounts)
-        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, QUEUE_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, QUEUE_COMPONENT_NAME) and CONTROLLER_TARGET_NAME not in exclude and QUEUE_COMPONENT_NAME not in exclude:
            start_queue(instance_id, clp_config)
-        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, REDIS_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, REDIS_COMPONENT_NAME) and CONTROLLER_TARGET_NAME not in exclude and REDIS_COMPONENT_NAME not in exclude:
            start_redis(instance_id, clp_config, conf_dir)
-        if target in (ALL_TARGET_NAME, RESULTS_CACHE_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, RESULTS_CACHE_COMPONENT_NAME) and RESULTS_CACHE_COMPONENT_NAME not in exclude:
            start_results_cache(instance_id, clp_config, conf_dir)
        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, RESULTS_CACHE_COMPONENT_NAME):
            create_results_cache_indices(instance_id, clp_config, container_clp_config, mounts)
        if target in (
            ALL_TARGET_NAME,
            CONTROLLER_TARGET_NAME,
            COMPRESSION_SCHEDULER_COMPONENT_NAME,
-        ):
+        ) and CONTROLLER_TARGET_NAME not in exclude and COMPRESSION_SCHEDULER_COMPONENT_NAME not in exclude:
            start_compression_scheduler(instance_id, clp_config, container_clp_config, mounts)
-        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, QUERY_SCHEDULER_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, CONTROLLER_TARGET_NAME, QUERY_SCHEDULER_COMPONENT_NAME) and CONTROLLER_TARGET_NAME not in exclude and QUERY_SCHEDULER_COMPONENT_NAME not in exclude:
            start_query_scheduler(instance_id, clp_config, container_clp_config, mounts)
-        if target in (ALL_TARGET_NAME, COMPRESSION_WORKER_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, COMPRESSION_WORKER_COMPONENT_NAME) and COMPRESSION_WORKER_COMPONENT_NAME not in exclude:
            start_compression_worker(
                instance_id, clp_config, container_clp_config, num_workers, mounts
            )
-        if target in (ALL_TARGET_NAME, QUERY_WORKER_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, QUERY_WORKER_COMPONENT_NAME) and QUERY_WORKER_COMPONENT_NAME not in exclude:
            start_query_worker(instance_id, clp_config, container_clp_config, num_workers, mounts)
-        if target in (ALL_TARGET_NAME, REDUCER_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, REDUCER_COMPONENT_NAME) and REDUCER_COMPONENT_NAME not in exclude:
            start_reducer(instance_id, clp_config, container_clp_config, num_workers, mounts)
-        if target in (ALL_TARGET_NAME, WEBUI_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, WEBUI_COMPONENT_NAME) and WEBUI_COMPONENT_NAME not in exclude:
            start_webui(instance_id, clp_config, mounts)
-        if target in (ALL_TARGET_NAME, LOG_VIEWER_WEBUI_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, LOG_VIEWER_WEBUI_COMPONENT_NAME) and LOG_VIEWER_WEBUI_COMPONENT_NAME not in exclude:
            start_log_viewer_webui(instance_id, clp_config, container_clp_config, mounts)


@haiqi96
These look simple but are definitely less clean and scalable. I am thinking about refactoring this into a dict which stores {component_name: (method, (args1, args2))} pairs and a for-loop to go over the dict. What do you think?

I feel it's ok to have the line that launches the component. the part that introduce complexity is the if statement, especially we have some component -> COMPONENT_NAME / GROUP_MAPPING but it's not very clear what are all the mappings. So I would prefer to approach this from simplifying the if statements.

Sure, I agree. Abstracting the if-condition checks into a single function call per if should keep the code simple and clean.

haiqi96 · 2024-07-24T00:40:00Z

components/clp-package-utils/clp_package_utils/scripts/start_clp.py

@@ -1096,37 +1098,37 @@ def main(argv):
        conf_dir = clp_home / "etc"

        # Start components
-        if target in (ALL_TARGET_NAME, DB_COMPONENT_NAME):
+        if target in (ALL_TARGET_NAME, DB_COMPONENT_NAME) and DB_COMPONENT_NAME not in exclude:


I personally don't like how the code is written now. it's quite hard to read and could be error prone. In addition, it looks like if user make a typo in the exclude list, CLP will sliently run the "exclude target"

In the closed source CLP, we have a config file to specify what components to run and one can remove component from the config file to remove components.
It might make more sense if we let the start-clp to launch everything by default (i.e. if a config file is not provided), or launch the specified component based on the config file.

If we plan to go with the current approach, I think if the if statement can be viewed as

if target in (ALL_TARGET, [TASK_TARGET]) and TASK_TARGET not in exclude:

maybe we can factor it out as a function to make the code less messy.

I personally don't like how the code is written now. it's quite hard to read and could be error prone. In addition, it looks like if user make a typo in the exclude list, CLP will sliently run the "exclude target"

In the closed source CLP, we have a config file to specify what components to run and one can remove component from the config file to remove components. It might make more sense if we let the start-clp to launch everything by default (i.e. if a config file is not provided), or launch the specified component based on the config file.

If we plan to go with the current approach, I think if the if statement can be viewed as

if target in (ALL_TARGET, [TASK_TARGET]) and TASK_TARGET not in exclude:

maybe we can factor it out as a function to make the code less messy.

With help from @Henry8192 , we have refactored the if-checks into calls of a checker helper. Also now we also validate whether the names in the exclude list are valid before proceeding. Let me know if this is more readable and maintainable now.

haiqi96 · 2024-07-29T19:36:29Z

components/clp-package-utils/clp_package_utils/scripts/start_clp.py

+    :param target: The target name specified by the user.
+    :param exclude: A list of component names that should be excluded from starting.
+    :param component_names: A tuple of components to which the target component corresponds
+                            or depends on.


Is there any example of "depends on"?

Ah I wrote "depends on" because I previously understood that "target"s (specially CONTROLLER_TARGET_NAME) are master components that contains subcomponents like DB, queue, redis, etc..

Now having understood the relationships better, I think it might make more sense to name this target_names or target_and_component_names. Let's discuss more about the glossary in thread https://github.com/y-scope/clp/pull/492/files#r1695767819

haiqi96 · 2024-07-29T19:37:01Z

Thanks for making the change, it now makes much more sense to me

haiqi96 · 2024-07-29T19:38:57Z

components/clp-package-utils/clp_package_utils/scripts/start_clp.py

@@ -59,6 +59,22 @@
    validate_worker_config,
 )

+# Constants
+COMPONENT_NAMES = (
+    CONTROLLER_TARGET_NAME,


Just a nit, a target isn't a component. One option is to change this list to COMPONENT_AND_TARGET_NAMES, or we can have two lists, one for individual components and another one for target(s)

Is it fair to say a component can always be a target? If so, maybe we can rename is as TARGET_NAMES and the usages below would be like we check the user specified "target" against this targets list.

Sure, I think I had this discussion with Kirk before and it's fair to call it TARGET_NAMES (so it is also consistent with ALL_TARGET_NAMES

Now when I look at the component_should_start, I kinda feel the function is a bit unintuitive and maybe can be simplified .. let me think if I can come up with somthing better

haiqi96 · 2024-08-07T15:33:21Z

Trying to think about this from another perspective, what if we rearrange the code, such that we have a function, takes in the target, the exclude, and generates a List: components_to_run. in that case, for each if statement, we simply need to write something like if DB_COMPONENT in components_to_run.

A trade off is that now for targets correspond to mulitple component, we would need to have some good way to define them.
Like

"ALL_TARGET": [
"comp1",
"comp2",
"comp3,
...
]
"CONTROLLER_TARGET": [
"comp1",
...
]

In the long term, this might introduce some clarity though. now it isn't very obvious from code that what components are in CONTROLLER_TARGET (like you have to find every if statement that has CONTROLLER_TARGET in it)

kirkrodrigues · 2024-11-13T12:31:19Z

What's the status of this PR?

haiqi96 · 2024-11-13T15:20:38Z

What's the status of this PR?

I guess Junhao got too busy on other stuff and this gets deprioritized

package: Add --exclude option to exclude components from being star…

1609ac4

…ted.

junhaoliao requested a review from haiqi96 July 23, 2024 20:43

junhaoliao commented Jul 23, 2024

View reviewed changes

haiqi96 reviewed Jul 24, 2024

View reviewed changes

junhaoliao added 3 commits July 27, 2024 12:45

Refactor component_should_start logics into a checker helper.

71bbed2

Check if component names are valid in --exclude list.

f9bbd19

Change list into tuple.

dd57e98

junhaoliao marked this pull request as ready for review July 27, 2024 17:15

Add period to prompt.

21ce25c

junhaoliao requested a review from haiqi96 July 29, 2024 16:15

haiqi96 reviewed Jul 29, 2024

View reviewed changes

junhaoliao requested a review from haiqi96 July 29, 2024 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

package: Add `--exclude` option to exclude components from being started. #492

package: Add `--exclude` option to exclude components from being started. #492

junhaoliao commented Jul 23, 2024

junhaoliao Jul 23, 2024

haiqi96 Jul 24, 2024

junhaoliao Jul 24, 2024 •

edited

Loading

haiqi96 Jul 24, 2024

junhaoliao Jul 27, 2024

haiqi96 Jul 29, 2024

junhaoliao Jul 29, 2024

haiqi96 commented Jul 29, 2024

haiqi96 Jul 29, 2024

junhaoliao Jul 29, 2024

haiqi96 Jul 29, 2024

haiqi96 Jul 29, 2024

haiqi96 commented Aug 7, 2024 •

edited

Loading

kirkrodrigues commented Nov 13, 2024

haiqi96 commented Nov 13, 2024

package: Add --exclude option to exclude components from being started. #492

Are you sure you want to change the base?

package: Add --exclude option to exclude components from being started. #492

Conversation

junhaoliao commented Jul 23, 2024

Description

Validation performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junhaoliao Jul 24, 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

haiqi96 commented Jul 29, 2024

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

haiqi96 commented Aug 7, 2024 • edited Loading

kirkrodrigues commented Nov 13, 2024

haiqi96 commented Nov 13, 2024

package: Add `--exclude` option to exclude components from being started. #492

package: Add `--exclude` option to exclude components from being started. #492

junhaoliao Jul 24, 2024 •

edited

Loading

haiqi96 commented Aug 7, 2024 •

edited

Loading