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.

Sign up for GitHub

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

Jump to bottom

Remove useless files for TiFlash #1596

Merged
merged 9 commits into from
Mar 24, 2021
Merged

Remove useless files for TiFlash #1596

merged 9 commits into from
Mar 24, 2021

Conversation

JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Mar 19, 2021 •
edited
Loading

What problem does this PR solve?

I want to make my editor index fewer files. There are many files not used by TiFlash.

What is changed and how it works?

Remove this submodule
contrib/capnproto

Remove these files

.gitlab-ci.yml
.travis.yml
CHANGELOG.md
CHANGELOG_RU.md
Jenkinsfile
MacOS.md
copy_headers.sh

debian/ -- tools for debian package
docker-compose.yml
docker/  -- dockefile for CK
website/ -- 
docs/     -- docs for CK
utils/      -- some utils for testing

release
release_lib.sh
dbms/benchmark/ -- benchmark with other dbms
dbms/scripts/ -- scripts  for generating test data

dbms/src/Server/clickhouse-client.xml
-- Some binaries that are useless for TiFlash
dbms/src/Server/ClusterCopier.cpp
dbms/src/Server/ClusterCopier.h
dbms/src/Server/clickhouse-copier.cpp
-- testing directories
dbms/src/Server/{data,metadata,users.d,config.d}/

Related changes

  • N/A

Check List

Tests

  • No code

Side effects

  • N/A

Release note

  • No release note

@JaySon-Huang JaySon-Huang self-assigned this Mar 19, 2021
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

1 similar comment
@tisonkun
Copy link
Contributor

/run-all-tests

zanmato1984
zanmato1984 reviewed Mar 22, 2021
View reviewed changes
dbms/src/Server/CMakeLists.txt

if (USE_EMBEDDED_COMPILER)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this option and the relative libs/code? We may investigate using codegen someday.

@flowbehappy flowbehappy self-requested a review March 22, 2021 03:53
flowbehappy
flowbehappy reviewed Mar 22, 2021
View reviewed changes
dbms/src/Server/CMakeLists.txt Outdated
add_library (clickhouse-client-lib Client.cpp)
target_link_libraries (clickhouse-client-lib clickhouse_functions clickhouse_aggregate_functions ${LINE_EDITING_LIBS} ${Boost_PROGRAM_OPTIONS_LIBRARY})
target_include_directories (clickhouse-client-lib PRIVATE ${READLINE_INCLUDE_DIR})
install (FILES clickhouse-client.xml DESTINATION ${CLICKHOUSE_ETC_DIR}/clickhouse-client COMPONENT clickhouse-client RENAME config.xml)

add_library (clickhouse-benchmark-lib ${SPLIT_SHARED} Benchmark.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep those "Benchmark.cpp, Compressor.cpp" files and by default do NOT compile them? I agree that we do not need them currently, but I think the idea here is very good, i.e. run benchmarks for each component separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/pingcap/tics/blob/6cee66f2a3ab93d909f83b7fef9a0ddbf894545b/dbms/src/Server/CMakeLists.txt#L6-L13
Yes, those binaries are not compiled if you run make tiflash

@JaySon-Huang
Clean
1597ef0 
Signed-off-by: JaySon-Huang <[email protected]>
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

1 similar comment
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Mar 22, 2021 •
edited
Loading

@zanmato1984 @flowbehappy I've restored those binary targets. PTAL again

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

zanmato1984
zanmato1984 approved these changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 24, 2021
@flowbehappy flowbehappy self-requested a review March 24, 2021 10:31
flowbehappy
flowbehappy reviewed Mar 24, 2021
View reviewed changes
Copy link
Contributor

@flowbehappy flowbehappy left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Mar 24, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 24, 2021
@JaySon-Huang
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 24, 2021
@ti-srebot
Copy link
Collaborator

Your auto merge job has been accepted, waiting for:

  • 1621

@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot
Copy link
Collaborator

@JaySon-Huang merge failed.

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

1 similar comment
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@JaySon-Huang
Copy link
Contributor Author

The test is running but shown as failure in Github. Waiting for it: https://internal.pingcap.net/idc-jenkins/blue/organizations/jenkins/tics_ghpr_test/detail/tics_ghpr_test/4219/pipeline/19

@JaySon-Huang JaySon-Huang merged commit cef4247 into pingcap:master Mar 24, 2021
@JaySon-Huang JaySon-Huang deleted the remove_useless branch March 24, 2021 15:03
@JaySon-Huang JaySon-Huang mentioned this pull request Mar 25, 2021
Merged
This was referenced May 28, 2021
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flowbehappy flowbehappy flowbehappy left review comments

@zanmato1984 zanmato1984 zanmato1984 approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees

@JaySon-Huang JaySon-Huang

Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JaySon-Huang @tisonkun @ti-srebot @zanmato1984 @flowbehappy