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

refactor(publisher): simplify the the publishing logic to fileserver #212

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Dec 16, 2024

BREAKING CHANGE:
the target file key is changed on the fileserver, now some of the file keys are:

  • tidb:
    • download/builds/pingcap/tidb/master/abc123def/linux_amd64/tidb.tar.gz
    • download/builds/pingcap/tidb/master/abc123def/linux_amd64/br.tar.gz
    • download/builds/pingcap/tidb/master/abc123def/linux_amd64/dumpling.tar.gz
    • download/builds/pingcap/tidb/master/abc123def/linux_amd64/tidb-lightning.tar.gz
    • download/builds/pingcap/tidb/master/abc123def/linux_amd64/tidb-lightning-ctl.tar.gz
  • tikv:
    • download/builds/tikv/tikv/master/abc123def/linux_amd64/linux_amd64/tikv.tar.gz",
    • download/builds/tikv/tikv/master/abc123def/linux_amd64/linux_amd64/tikv-ctl.tar.gz",
      ......

target ref keys are:

  • tidb: download/refs/pingcap/tidb/master/sha1
  • tikv: download/refs/tikv/tikv/master/sha1

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind December 16, 2024 11:47
@ti-chi-bot ti-chi-bot bot added the size/XXL label Dec 16, 2024
Copy link

ti-chi-bot bot commented Dec 16, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary

The pull request refactors the publishing logic to the file server with the following key changes:

  1. A new field FileTransferMap is added in PublishInfoFS struct, which is a map to hold the file transfer information for each file.
  2. The analyzeFsFromOciArtifact function is refactored to handle the new PublishInfoFS struct instead of PublishInfo.
  3. The handle function in tiupWorker and fsWorker have been updated to handle the new PublishRequestFS and PublishRequestTiUP respectively.
  4. The notifyLark function in tiupWorker have been updated to handle the new PublishRequestTiUP.
  5. Functions like targetFsFullPaths, targetFsRefKeyValue etc. have been updated to handle the new PublishInfoFS.
  6. Unit tests have been updated to reflect the above changes.

Potential Problems

  1. The changes in pull request are marked as a breaking change, which could potentially disrupt existing usage.
  2. The refactoring might have unintended side effects, if not all usage cases are covered in the testing.
  3. The refactoring could potentially introduce bugs if the changes in the data structures are not handled properly in all parts of the codebase.

Suggestions

  1. Ensure that the changes are properly communicated to all stakeholders.
  2. Increase test coverage to ensure all edge cases are covered.
  3. Make sure to perform thorough regression testing to ensure the refactoring does not introduce new bugs.

BREAKING CHANGE:
the target file key is changed on the fileserver, now some of the file keys are:

- tidb:
  - download/builds/pingcap/tidb/master/abc123def/linux_amd64/tidb.tar.gz
  - download/builds/pingcap/tidb/master/abc123def/linux_amd64/br.tar.gz
  - download/builds/pingcap/tidb/master/abc123def/linux_amd64/dumpling.tar.gz
  - download/builds/pingcap/tidb/master/abc123def/linux_amd64/tidb-lightning.tar.gz
  - download/builds/pingcap/tidb/master/abc123def/linux_amd64/tidb-lightning-ctl.tar.gz
- tikv:
  - download/builds/tikv/tikv/master/abc123def/linux_amd64/linux_amd64/tikv.tar.gz",
  - download/builds/tikv/tikv/master/abc123def/linux_amd64/linux_amd64/tikv-ctl.tar.gz",
......

Signed-off-by: wuhuizuo <[email protected]>
@wuhuizuo wuhuizuo force-pushed the feature/add-ctl-uploads branch from 74db4b9 to f6cb705 Compare December 16, 2024 12:02
Copy link

ti-chi-bot bot commented Dec 16, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

This is a large refactor that simplifies the logic for publishing to a file server. The main changes are:

  1. The structure PublishRequest is replaced by two structures PublishRequestTiUP and PublishRequestFS to differentiate between TiUP publish requests and FS publish requests. Similar changes are made to PublishInfo structure.

  2. The method analyzeFsFromOciArtifact has been updated to return *PublishRequestFS instead of []PublishRequest. It also includes changes to fetch artifact config, get file keys and map them accordingly.

  3. The handle method in fsWorker and tiupWorker now accepts *PublishRequestFS and *PublishRequestTiUP respectively.

  4. The method notifyLark in fsWorker and tiupWorker now accepts *PublishRequestFS and *PublishRequestTiUP respectively.

  5. The methods composeEvents in both tiup_service.go and fileserver_service.go have been updated to use the new types.

  6. The function targetFsFullPaths has been updated to return a map of file keys and their full paths.

  7. The function getFileKeys is added to get file keys from a list of files.

  8. The method publish in fsWorker and tiupWorker has been refactored.

  9. Few test cases have been updated to reflect these changes.

Potential Problems:

  1. The change is quite large and might have missed some edge cases.

  2. The handling of error conditions and exceptions might have been changed, which could introduce issues.

Fixing suggestions:

  1. Ensure extensive testing is done to cover all edge cases.

  2. Review the error handling and exception handling code to ensure it is working as expected.

  3. Consider splitting such a large change into smaller parts to make it easier to review and test.

  4. Update the test cases to reflect these changes and ensure they are passing.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Dec 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Dec 17, 2024
Copy link

ti-chi-bot bot commented Dec 17, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

This Pull Request refactors the publishing logic to the file server. The changes include:

  1. The PublishRequest struct has been separated into two different structs: PublishRequestTiUP and PublishRequestFS, each for handling a specific type of request.

  2. The PublishInfo struct has been separated into PublishInfoTiUP and PublishInfoFS, each for handling a specific type of publishing info.

  3. The analyzeFsFromOciArtifact and analyzeFsFromOciArtifactUrl functions have been updated to return *PublishRequestFS instead of []PublishRequest.

  4. The handle function in tiup_worker.go has been updated to take a *PublishRequestTiUP parameter instead of *PublishRequest.

  5. The notifyLark function in tiup_worker.go has been updated to take a *PublishRequestTiUP parameter instead of *PublishRequest.

Potential Problems:

  1. There could be potential issues with backward compatibility because of the changes in the structs. If there are other parts of the code or services that are using the old PublishRequest and PublishInfo, they would have to be updated to use the new structs.

Fixing Suggestions:

  1. To avoid backward compatibility issues, you could create new structs instead of renaming and changing the existing ones. This way, the existing code that uses the old structs will not be affected.

  2. Ensure that all parts of the code that use these structs are updated with the new structs. This includes any tests, mocks, or interfaces that use these structs.

  3. Add unit tests for the new structs and functions to ensure that they work as expected.

  4. Test these changes in a controlled environment before deploying to production to ensure that nothing breaks.

@ti-chi-bot ti-chi-bot bot merged commit 492a0b1 into main Dec 17, 2024
7 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-ctl-uploads branch December 17, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant