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

txn: seperate prewrite and commit in detail information and add clone methods #567

Merged
merged 2 commits into from
Aug 18, 2022

Conversation

cfzjywxk
Copy link
Contributor

Ref: tikv/tikv#12362

As the CommitDetails is changed and it's directly used in the tidb code, this is a breaking change.

  • Separate the prewrite and commit related details in execDetail. So it's more clear for the 2pc mode committed transactions.
{
   "prewrite":14.6ms,
   "get_commit_ts":521.9µs,
   "commit":6.67ms,
   "slowest_prewrite_rpc":{
      "total":0.015s,
      "region_id":2,
      "store":"127.0.0.1":20160,
      "tikv_wall_time":13.4ms,
      "scan_detail":{
         "get_snapshot_time":409µs,
         "rocksdb":{
            "block":{
               
            }
         }
      },
      "write_detail":{
         "store_batch_wait":246.3µs,
         "propose_send_wait":0s,
         "persist_log":{
            "total":8.82ms,
            "write_leader_wait":5.39µs,
            "sync_log":7.83ms,
            "write_memtable":76.1µs
         },
         "commit_log":9.31ms,
         "apply_batch_wait":179.8µs,
         "apply":{
            "total":1.01ms,
            "mutex_lock":654ns,
            "write_leader_wait":0s,
            "write_wal":179.8µs,
            "write_memtable":33.7µs
         }
      }
   },
   "commit_primary_rpc":{
      "total":0.007s,
      "region_id":2,
      "store":"127.0.0.1":20160,
      "tikv_wall_time":5.58ms,
      "scan_detail":{
         "get_snapshot_time":326µs,
         "rocksdb":{
            "block":{
               
            }
         }
      },
      "write_detail":{
         "store_batch_wait":292.6µs,
         "propose_send_wait":0s,
         "persist_log":{
            "total":1.72ms,
            "write_leader_wait":3.82µs,
            "sync_log":998.1µs,
            "write_memtable":51.3µs
         },
         "commit_log":2.08ms,
         "apply_batch_wait":146.6µs,
         "apply":{
            "total":799.7µs,
            "mutex_lock":625ns,
            "write_leader_wait":0s,
            "write_wal":146.6µs,
            "write_memtable":37.7µs
         }
      }
   },
   "region_num":1,
   "write_keys":1,
   "write_byte":29
}
  • Implement the actual Clone methods for several structs.

Signed-off-by: cfzjywxk lsswxrxr@163.com

Copy link
Contributor

@zyguan zyguan left a comment

Choose a reason for hiding this comment

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

LGTM

// The prewrite requests are executed concurrently so the slowest request information would be recorded.
SlowestPrewrite ReqDetailInfo
// It's recorded only when the commit mode is 2pc.
CommitPrimary ReqDetailInfo
Copy link
Contributor

@zyguan zyguan Aug 17, 2022

Choose a reason for hiding this comment

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

Shall we also observe the number of prewrite/commit requests as well?

Copy link
Contributor Author

@cfzjywxk cfzjywxk Aug 17, 2022

Choose a reason for hiding this comment

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

A new field PrewriteReqNum is added.

@sticnarf
Copy link
Collaborator

sticnarf commented Aug 17, 2022

Where are the deep clones used? I'm afraid deep cloning such deep nested structures could be very expensive and if we really need to use it, it's probably unacceptable in performance

@cfzjywxk
Copy link
Contributor Author

Where are the deep clones used? I'm afraid deep cloning such deep nested structures could be very expensive and if we really need to use it, it's probably unacceptable in performance

@sticnarf
By now clone happens when the binary plan is generated or the slow log information is printed, I'll verify it.
The tidb runtime-stat-related utilities use clone in many places but some of them are not clone actually, this is another risk in the current runtime-stat framework.

Signed-off-by: cfzjywxk <lsswxrxr@163.com>
@cfzjywxk
Copy link
Contributor Author

The CommitDetail may have just one commit information related typed object in tidb RootRuntime statistics, so it would not be cloned normally, the clone related changes are reverted.

Copy link
Collaborator

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM. The tidb in integration tests have to be upgraded to make CI pass.

Signed-off-by: cfzjywxk <lsswxrxr@163.com>
@cfzjywxk
Copy link
Contributor Author

@sticnarf
As it's a breaking change the compatible test could not pass maybe we could merge it with tidb pr first?

@sticnarf
Copy link
Collaborator

@sticnarf As it's a breaking change the compatible test could not pass maybe we could merge it with tidb pr first?

The compatibility test is not required. Let me retry the failed integration test.

@cfzjywxk
Copy link
Contributor Author

@sticnarf As it's a breaking change the compatible test could not pass maybe we could merge it with tidb pr first?

The compatibility test is not required. Let me retry the failed integration test.

It seems the integration test needs to build tidb like the compatibility test.

Error: ../../../../go/pkg/mod/github.com/defined2014/tidb@v1.1.0-beta.0.20220725074011-ee134690c221/util/execdetails/execdetails.go:162:27: commitDetails.Mu.BackoffTypes undefined (type struct{sync.Mutex; CommitBackoffTime int64; PrewriteBackoffTypes []string; CommitBackoffTypes []string; SlowestPrewrite util.ReqDetailInfo; CommitPrimary util.ReqDetailInfo} has no field or method BackoffTypes)

@sticnarf sticnarf merged commit 0d0ae0d into tikv:master Aug 18, 2022
disksing pushed a commit to oh-my-tidb/client-go that referenced this pull request Dec 14, 2022
… methods (tikv#567)

* seperate the prewrite and commit primary in runtime info

Signed-off-by: cfzjywxk <lsswxrxr@163.com>

* fix the integration test

Signed-off-by: cfzjywxk <lsswxrxr@163.com>

Signed-off-by: cfzjywxk <lsswxrxr@163.com>
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.

3 participants