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

Support grpc forward #198

Merged
merged 12 commits into from
Jun 18, 2021
Merged

Support grpc forward #198

merged 12 commits into from
Jun 18, 2021

Conversation

Little-Wallace
Copy link
Contributor

What problem does this PR solve?

When TiKV cluster deploy in three IDC A/B/C, if there is network isolation between the A and B, the message which should be send to leader in A can be forward by B.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@@ -72,6 +75,7 @@ private static void loadFromDefaultProperties() {
setIfMissing(TIKV_METRICS_ENABLE, DEF_METRICS_ENABLE);
setIfMissing(TIKV_METRICS_PORT, DEF_METRICS_PORT);
setIfMissing(TIKV_NETWORK_MAPPING_NAME, DEF_TIKV_NETWORK_MAPPING_NAME);
setIfMissing(TIKV_ENABLE_GRPC_FORWARD, false);

Choose a reason for hiding this comment

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

Looks like you'd better define a DEF_ENABLE_GRPC_FORWARD

if (checkHealth(store)) {
return true;
} else {
store.invalid();

Choose a reason for hiding this comment

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

Does the function invalid actually means invalidate?

@@ -32,6 +33,8 @@

private static final Logger logger = LoggerFactory.getLogger(TiConfiguration.class);
private static final ConcurrentHashMap<String, String> settings = new ConcurrentHashMap<>();
public static final Metadata.Key FORWARD_META_DATA_KEY =

Choose a reason for hiding this comment

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

Suggested change
public static final Metadata.Key FORWARD_META_DATA_KEY =
public static final Metadata.Key FORWARD_METADATA_KEY =

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Copy link
Collaborator

@birdstorm birdstorm left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@@ -76,6 +77,7 @@
public static final boolean DEF_METRICS_ENABLE = false;
public static final int DEF_METRICS_PORT = 3140;
public static final String DEF_TIKV_NETWORK_MAPPING_NAME = "";
public static final boolean DEF_GRPC_FORWARD_ENABLE = true;

Choose a reason for hiding this comment

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

Why enable by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not?

Copy link

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

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

Rest LGTM, but I'm not very confident. Please confirm it works by tests.

Comment on lines 28 to 35
TiStore oldStore = this.stores.get(Long.valueOf(store.getId()));
if (oldStore != null) {
return;
}
synchronized (this.taskQueue) {
this.stores.put(Long.valueOf(store.getId()), store);
this.taskQueue.add(store);
}

Choose a reason for hiding this comment

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

Is this function possible to be called concurrently? May it be a race that multiple threads do the check-and-insert-if-not-exist operation at the same time?

continue;
}
} finally {
}

Choose a reason for hiding this comment

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

What's the purpose of the try-finally?

List<TiStore> unhealthStore = getUnhealthStore();
List<TiStore> restStore = new LinkedList<>();
for (TiStore store : unhealthStore) {
String addressStr = store.getStore().getAddress();

Choose a reason for hiding this comment

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

Did you check if the store becomes tombstone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the store will not be tombstone after it was created. Because it is a final object.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
@Little-Wallace Little-Wallace merged commit 5815678 into tikv:master Jun 18, 2021
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Jun 23, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.1 in PR #208

birdstorm pushed a commit to ti-srebot/client-java that referenced this pull request Jun 23, 2021
* support grpc forward for tikv client

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
birdstorm pushed a commit to ti-srebot/client-java that referenced this pull request Jun 23, 2021
* support grpc forward for tikv client

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: birdstorm <samuelwyf@hotmail.com>
birdstorm added a commit that referenced this pull request Jun 23, 2021
* Support grpc forward (#198)

* support grpc forward for tikv client

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Signed-off-by: birdstorm <samuelwyf@hotmail.com>

* fix compile

Signed-off-by: birdstorm <samuelwyf@hotmail.com>

Co-authored-by: Wallace <bupt2013211450@gmail.com>
Co-authored-by: birdstorm <samuelwyf@hotmail.com>
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.

None yet

4 participants