-
Notifications
You must be signed in to change notification settings - Fork 526
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
Add MySQL protocol layer code #97
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. Just remind that we're developing a course instead of yet another TiDB, so please narrow the code shape as much as possible.
.gitignore
Outdated
# Binaries for programs and plugins | ||
*.exe | ||
*.exe~ | ||
*.dll | ||
*.so | ||
*.dylib | ||
|
||
# Test binary, built with `go test -c` | ||
*.test | ||
|
||
# Output of the go coverage tool, specifically when used with LiteIDE | ||
*.out | ||
|
||
# Dependency directories (remove the comment below to include it) | ||
# vendor/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any of these lines necessary.
Makefile
Outdated
|
||
include Makefile.common | ||
|
||
.PHONY: all clean test gotest server dev benchkv benchraw check checklist parser tidy ddltest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unnecessary items at the moment.
Makefile
Outdated
# Ensure GOPATH is set before running build process. | ||
ifeq "$(GOPATH)" "" | ||
$(error Please set the environment variable GOPATH before running `make`) | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GOPATH ?= $(shell go env GOPATH)
already ensures this?
PROJECT=tinysql | ||
GOPATH ?= $(shell go env GOPATH) | ||
P=8 | ||
|
||
# Ensure GOPATH is set before running build process. | ||
ifeq "$(GOPATH)" "" | ||
$(error Please set the environment variable GOPATH before running `make`) | ||
endif | ||
FAIL_ON_STDOUT := awk '{ print } END { if (NR > 0) { exit 1 } }' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should remove repeats in Makefile
?
Makefile.common
Outdated
GOTEST := $(GO) test -p $(P) | ||
OVERALLS := GO111MODULE=on overalls | ||
STATICCHECK := GO111MODULE=on staticcheck | ||
TIDB_EDITION ?= Community |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or please tidy Makefile.common
to what we exactly want. At least I don't think we should keep this TiDB related stuff especially about edition.
@tisonkun Thanks for your review. I totally agree with you. At present, the MySQL protocol layer code is still too large. There are two main reasons:
The reason for doing this at present is that these two parts are not actually the focus of the TinySQL course. And these two jobs are not related to subsequent courses. So I took them down as issue and hoped to finish it later. Developing and submitting labs have a higher priority, I hope we can focus on them first. |
7d1df07
to
402ada8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost looks good. Please resolve outstanding comments.
Makefile
Outdated
|
||
default: server buildsucc | ||
|
||
PROJECT=tinysql | ||
GOPATH ?= $(shell go env GOPATH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may still keep this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But well. It's OK to merge. If someone wants to tolerate it, file an issue.
This closes #88
This PR contains the simple MySQL protocol layer code. These codes refer to the implementation of TiDB, but are simplified and only include the necessary implementation.
It should be noted that the current implementation only supports the "mysql_native_password" auth plugin.
How to use
compile and run
use MySQL-client
TODO