Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Doc] move docker compose related documents to StarRocks/demo #34883

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

yandongxiao
Copy link
Contributor

@yandongxiao yandongxiao commented Nov 13, 2023

Why I'm doing:

What I'm doing:

move docker compose related documents to StarRocks/demo project

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.2
    • 3.1
    • 3.0
    • 2.5

```

Replace `<be_ip>` with the actual IP address of the BE node.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most risky bug in this code is:
Possible inclusion of an actively loading external image with specific tracking identifiers in the initial HTML comment block.

You can modify the code like this:

<!-- Removed potentially privacy-compromising external image reference with tracking ID -->

Please note that while the provided diff output includes various textual changes, structural YAML changes, and comments on proper usage of Docker Compose for deploying StarRocks, code bugs and their risk cannot be properly assessed without additional context. Comments within markdown files are not executable code but the external image with a tracking ID could have implications for user privacy if this was part of a larger code base where the image could be loaded.


volumes:
singleton_fe0_data:
singleton_be0_data:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most risky bug in this code is:
There is a discrepancy in the service dependency name used within depends_on: section for the starrocks-be-0 service.

You can modify the code like this:

@@ -1,36 +1,46 @@
 version: "3.9"
 services:
   starrocks-fe-0:
     # ... other configurations stay the same

   starrocks-be-0:
     image: starrocks/be-ubuntu:latest
     hostname: starrocks-be-0
     command:
       - /bin/bash
       - -c
       - |
         /opt/starrocks/be_entrypoint.sh starrocks-fe-0
     environment:
       - HOST_TYPE=FQDN
       - TZ=Asia/Shanghai
     ports:
       - "8040:8040"
     depends_on:
       - starrocks-fe-0  # Corrected from starrocks-fe to starrocks-fe-0
     volumes:
       - singleton_be0_data:/opt/starrocks/be/storage
 
 volumes:
   singleton_fe0_data:
   singleton_be0_data:

This change ensures that starrocks-be-0 correctly identifies its dependency on starrocks-fe-0 as declared at the beginning of the services configuration.

1fe3be_fe0_data:
1fe3be_be0_data:
1fe3be_be1_data:
1fe3be_be2_data:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most risky bug in this code is:
The modified Docker Compose file seems to have removed the container_name settings but hasn't updated the depends_on configuration to match the updated service names.

You can modify the code like this:

@@ -1,67 +1,73 @@
 version: "3.9"
 services:
-  starrocks-fe:
+  starrocks-fe-0:
     # ... previous configuration remains unchanged ...

     depends_on:  # This is where the change needs to happen
-      - starrocks-be-0
+      - starrocks-fe-0  # This needs to be the current service's actual name
       - starrocks-be-0  # All similar lines depending on `starrocks-fe` should be updated
       - starrocks-be-1  
       - starrocks-be-2

+volumes:
+  1fe3be_fe0_data:
+  1fe3be_be0_data:
+  1fe3be_be1_data:
+  1fe3be_be2_data:

Remember that this configuration suggests either you keep the existing references in depends_on clauses to the old service name, reassuring it's defined somewhere or correctly rename those entries to match potentially renamed services if that was the intent.

image: starrocks/be-ubuntu:latest
#user: root
hostname: starrocks-be-2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although the container_name is not required, maybe it is good to specify the container_name the same as the service?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@imay
Copy link
Contributor

imay commented Nov 13, 2023

@yandongxiao please fill the commit message with the background, then others will know what you are doing in this CL.

@yandongxiao yandongxiao force-pushed the bugfix/docker-compose-files branch from d772e36 to 1ba9cbb Compare November 14, 2023 01:59
@yandongxiao yandongxiao force-pushed the bugfix/docker-compose-files branch 2 times, most recently from 895863c to 71bb763 Compare November 14, 2023 04:40
@yandongxiao yandongxiao changed the title [Bugfix] Fix starrocks cluster failed when docker compose restart [Doc] move docker compose related documents to StarRocks/demo Nov 14, 2023
@yandongxiao yandongxiao force-pushed the bugfix/docker-compose-files branch from 71bb763 to 4d2c297 Compare November 14, 2023 04:52
Signed-off-by: yandongxiao <dxyan06@gmail.com>
@yandongxiao yandongxiao force-pushed the bugfix/docker-compose-files branch from 4d2c297 to 7e9a026 Compare November 14, 2023 04:53
Copy link

[FE Incremental Coverage Report]

pass : 0 / 0 (0%)

Copy link

[BE Incremental Coverage Report]

pass : 0 / 0 (0%)

@wyb wyb merged commit ab2b465 into StarRocks:main Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants