Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

*: avoid accessing internal ports when backend=tidb #312

Merged
merged 2 commits into from
May 11, 2020

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented May 10, 2020

What problem does this PR solve?

Fix #306.

The PD address port (:2379) and TiDB status port (:10080) by default are not exposed to public. In TiDB-Backend mode, we may not be able to access these ports.

What is changed and how it works?

Eliminated access of these two ports in TiDB backend by:

  • Check-requirements are now a backend-specific operation, and is a no-op in TiDB backend (the "local" backend WIP backend: add local backend #294 should check the the TiKV version is 4.0.0, BTW.)
  • The TiDB backend only needs to know the column names, so fetching the entire TableInfo is unnecessary. Moved the "fetch table info" operation into backend as well, where the TiDB backend simply fill in the column names from INFORMATION_SCHEMA.

Check List

Tests

  • Unit test

Side effects

Related changes

  • Need to be included in the release note
    • Lightning in TiDB-backend no longer requires access to PD or the TiDB status port. Thus, Lightning in TiDB-backend can be executed outside of the cluster if desired.

@kennytm kennytm added priority/normal status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring labels May 10, 2020
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels May 11, 2020
@kennytm kennytm requested a review from 3pointer May 11, 2020 07:22
Copy link
Contributor

@3pointer 3pointer left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu merged commit c10636e into master May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT1 One reviewer already commented LGTM (LGTM1) type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support automatically skipping version check when pd-url is not given
4 participants