feat: Add support for proper exception handling#1694
Conversation
kavirajk
force-pushed
the
kavirajk/exception-tag-support
branch
2 times, most recently
from
1b39743 to
3ba8b58
Compare
Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
Signed-off-by: Kaviraj <[email protected]>
kavirajk
force-pushed
the
kavirajk/exception-tag-support
branch
from
4e5ab72 to
6ae160a
Compare
conn_http.go
Outdated
| if n > 0 && captureBuffer != nil { | ||
| captureBuffer.Write(buf[:n]) | ||
| } | ||
| if readErr != nil { |
There was a problem hiding this comment.
I'd add some debug log here - when we failed, error
conn_http.go
Outdated
|
|
||
| // Try to read any remaining data | ||
| lr := &limitedReader{reader: reader, limit: 100 * 1024} | ||
| buf := make([]byte, 4096) |
There was a problem hiding this comment.
it would require 25 times to call socket IO.
I think this can be increased to 10024 safely.
There was a problem hiding this comment.
Good catch. Thanks.
I actually made a small change to it. Given the max exception block is 16KiB and when exception happens at the mid-stream, CH flushes the existing chunks (blocks) and start new one just for the exception.
So the exception would be the only thing present in this block. So just allocating 16KiB and reading it once should be good enough. Updated accordingly.
conn_http.go
Outdated
| // "__exception__" marker as binary data | ||
|
|
||
| // Try to read any remaining data | ||
| lr := &limitedReader{reader: reader, limit: 100 * 1024} |
There was a problem hiding this comment.
This relates to the block size. What if block size is 1Mb and exception is at the end?
conn_http.go
Outdated
| dataStr := string(data) | ||
|
|
||
| // Verify exception marker exists | ||
| if !strings.Contains(dataStr, "__exception__") { |
There was a problem hiding this comment.
this is redundant because next if checks it again using proper way.
There was a problem hiding this comment.
good catch. Removed it.
conn_http.go
Outdated
| } | ||
|
|
||
| // Skip the exception tag (16 bytes) + \r\n | ||
| if pos+16+2 < len(dataStr) { |
There was a problem hiding this comment.
16 looks very megical - lets create a constant what ideally should be calculated from marker string (what we actually do a few lines above.
There was a problem hiding this comment.
Make sense. Moved it to local constants :)
conn_http.go
Outdated
| } | ||
|
|
||
| // Skip past first __exception__\r\n | ||
| pos := firstMarker + len("__exception__") |
There was a problem hiding this comment.
we can save len("__exception__") for further use.
There was a problem hiding this comment.
Make sense. Moved it to local constants as well :)
1. Move magic numbers to well defined constants 2. Fix the block size when decoding Signed-off-by: Kaviraj <[email protected]>
2 participants
Summary
Related: #1468
This PR adds support to extract the exception happens during mid-stream. Supports generic exception marker for both CH 24.11 and before.
Checklist
Delete items not relevant to your PR: