Fix pserver checkpoint #5102
Fix pserver checkpoint #5102
Conversation
The pserver checkpoint before failed because the MD5 checksum is calculated incorrectly. Now changed to CRC32 checksum.
go/pserver/optimizer.go
Outdated
| var cptr (*C.uchar) | ||
| if len(c) > 0 { | ||
| cptr = (*C.uchar)(&c[0]) | ||
| } |
There was a problem hiding this comment.
Else output error log?
| h := md5.New() | ||
| md5 := hex.EncodeToString(h.Sum(content)) | ||
| if md5 != cpMeta.MD5 { | ||
| crc32 := crc32.ChecksumIEEE(content) |
There was a problem hiding this comment.
Curious why md5 will cause the error?
There was a problem hiding this comment.
@typhoonzero @Yancey1989 @dzhwinter
The problem was I think we used the md5 package incorrectly that generated a long string, causes etcd write error with "message too large". To show to error:
package main
import (
"crypto/md5"
"encoding/hex"
"fmt"
)
func main() {
h := md5.New()
md5 := hex.EncodeToString(h.Sum([]byte("hello this is some string")))
// Output: 68656c6c6f207468697320697320736f6d6520737472696e67d41d8cd98f00b204e9800998ecf8427e
fmt.Println(md5)
}The output is not a typical MD5 string, rather a very long one.
I think the correct way to get the MD5 string is here:
package main
import (
"crypto/md5"
"fmt"
)
func main() {
data := []byte("These pretzels are making me thirsty.")
sum := fmt.Sprintf("%x", md5.Sum(data))
// Output: b0804ec967f48520697662a204f5fe72
fmt.Printf(sum)
}The reason to switch to CRC32 is because it's faster, better for checksum, MD5 is slower, better for defending cracking.
| h := md5.New() | ||
| md5 := hex.EncodeToString(h.Sum(content)) | ||
| if md5 != cpMeta.MD5 { | ||
| crc32 := crc32.ChecksumIEEE(content) |
There was a problem hiding this comment.
one more question. Why we change MD5 to CRC, concern the speed?
| // successful. | ||
| log.Error("remove old meta file error", log.Ctx{"error": rmErr}) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
Maybe we would add some log here, sorry this is out of this PR code.
There was a problem hiding this comment.
The caller will handle it, here is one caller code:
err := s.checkpoint()
if err != nil {
log.Error("checkpoint error", log.Ctx{"error": err})
}I think in general the outer most caller should handle the error (either log or do something else), because it has the most information. If everyone prints log, it will be duplicating.
| h := md5.New() | ||
| md5 := hex.EncodeToString(h.Sum(content)) | ||
| if md5 != cpMeta.MD5 { | ||
| crc32 := crc32.ChecksumIEEE(content) |
The pserver checkpoint before failed because the MD5 checksum is
calculated incorrectly. Now changed to CRC32 checksum.