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

*: write binlog to unix socket. #1660

Merged
merged 4 commits into from
Sep 21, 2016
Merged

*: write binlog to unix socket. #1660

merged 4 commits into from
Sep 21, 2016

Conversation

coocood
Copy link
Member

@coocood coocood commented Aug 30, 2016

No description provided.

@coocood
Copy link
Member Author

coocood commented Aug 30, 2016

@shenli @disksing @iamxy @qiuyesuifeng PTAL

@disksing
Copy link
Contributor

Do we need a switch to turn on/off this feature? Not all user need it.

"github.com/pingcap/tidb/table"
"github.com/pingcap/tidb/terror"
// import table implementation to init table.TableFromMeta
"github.com/pingcap/tidb/perfschema"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any special reason to move the import?

@coocood
Copy link
Member Author

coocood commented Aug 31, 2016

@disksing @hanfei1991 PTAL

@@ -515,7 +525,38 @@ func (t *Table) RemoveRecord(ctx context.Context, h int64, r []types.Datum) erro
if err != nil {
return errors.Trace(err)
}

if binloginfo.Enable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here can use a function.

@coocood coocood changed the title *: save binlog when commit [DNM]*: save binlog when commit Sep 1, 2016
// See the License for the specific language governing permissions and
// limitations under the License.

package binloginfo
Copy link
Member

Choose a reason for hiding this comment

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

How about rename it to binlog?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then the package name will conflict with tipb/go-binlog.

@shenli
Copy link
Member

shenli commented Sep 10, 2016

For same statements, we use another sql to do some sub jobs. For example, set global variable, it will run an update statement. Do we need to avoid add an update binlog when set global variable?

@coocood coocood force-pushed the coocood/add-binlog-event branch from f4c066f to 1fdbfa5 Compare September 13, 2016 09:09
@coocood coocood changed the title [DNM]*: save binlog when commit *: write binlog to unix socket. Sep 13, 2016
@coocood
Copy link
Member Author

coocood commented Sep 13, 2016

@iamxy @shenli @zimulala PTAL

@coocood coocood force-pushed the coocood/add-binlog-event branch from a62b19f to 17a03f2 Compare September 18, 2016 03:25
@coocood
Copy link
Member Author

coocood commented Sep 18, 2016

@iamxy @shenli @zimulala PTAL

@coocood coocood force-pushed the coocood/add-binlog-event branch from 78c7fa8 to 6c68530 Compare September 18, 2016 07:56
@@ -184,18 +185,29 @@ func (s *session) finishTxn(rollback bool) error {
if s.txn == nil {
return nil
}
sessVar := variable.GetSessionVars(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

sessVar seems useless.

req := &binlog.WriteBinlogReq{ClusterID: binloginfo.ClusterID, Payload: rollbackData}
resp, err := binloginfo.PumpClient.WriteBinlog(context.Background(), req)
if err != nil {
log.Error(err)
Copy link
Contributor

@zimulala zimulala Sep 18, 2016

Choose a reason for hiding this comment

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

Add more log information. The same as the line 431.

err = errors.New(resp.Errmsg)
}
}
ch <- err
Copy link
Contributor

Choose a reason for hiding this comment

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

err -> errors.Trace(err) ?

@coocood
Copy link
Member Author

coocood commented Sep 20, 2016

@shenli @zimulala PTAL

Copy link
Member

@shenli shenli 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

}

func (c *txnCommitter) writeCommitBinlog() {
if binloginfo.PumpClient == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Use shouldWriteBinlog?

}

func (c *txnCommitter) rollbackBinlog() {
if binloginfo.PumpClient == nil {
Copy link
Member

Choose a reason for hiding this comment

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

shouldWriteBinlog?

return nil
}

func (c *txnCommitter) prewriteBinlog() chan error {
if binloginfo.PumpClient == nil {
Copy link
Member

Choose a reason for hiding this comment

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

shouldWriteBinlog?

support write binlog to a unix socket through grpc.
@coocood coocood force-pushed the coocood/add-binlog-event branch from 482b454 to af7b814 Compare September 20, 2016 10:22
log.Fatal(errors.ErrorStack(err1))
}
binloginfo.ClusterID = id
}
Copy link
Contributor

@zimulala zimulala Sep 20, 2016

Choose a reason for hiding this comment

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

Can line 101-112 be extract into a function?I think simpler and shorter code in the function of main is better. The same as line 122-130.

if err != nil {
log.Fatal(errors.ErrorStack(err))
}
if cluster := u.Query().Get("cluster"); cluster != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

If cluster == "" it has some problems?

return errors.Trace(err)
}
log.Warnf("txn commit succeed with error: %v, tid: %d", err, c.startTS)
}
c.writeCommitBinlog()
Copy link
Contributor

@zimulala zimulala Sep 20, 2016

Choose a reason for hiding this comment

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

Why not put c.writeCommitBinlog() and c.rollbackBinlog() in tikvTxn.Commit()?

@coocood coocood force-pushed the coocood/add-binlog-event branch from def9152 to 5cd4845 Compare September 20, 2016 11:07
PrewriteKey: c.keys[0],
PrewriteValue: prewriteValue.([]byte),
}
prewriteData, _ := binPrewrite.Marshal()
Copy link
Contributor

@zimulala zimulala Sep 21, 2016

Choose a reason for hiding this comment

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

Check the error. The same as line 418 and line 442

go func() {
prewriteValue := c.txn.us.GetOption(kv.BinlogData)
var err error
if prewriteValue != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the return value of c.shouldWriteBinlog() is ture, the value of prewriteValue != nil also is true.

}
prewriteData, _ := binPrewrite.Marshal()
req := &binlog.WriteBinlogReq{ClusterID: binloginfo.ClusterID, Payload: prewriteData}
var resp *binlog.WriteBinlogResp
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be removed.

@zimulala
Copy link
Contributor

LGTM

@coocood coocood merged commit 1dd9e89 into master Sep 21, 2016
@coocood coocood deleted the coocood/add-binlog-event branch September 21, 2016 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants