-
Notifications
You must be signed in to change notification settings - Fork 705
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
fix repl always return 0 #1286
fix repl always return 0 #1286
Conversation
@@ -81,15 +81,13 @@ func render(rsp interface{}, table *tablewriter.Table) bool { | |||
table.Append(row) | |||
isTable = true | |||
case error: | |||
if os.Getenv("SQLFLOW_log_dir") != "" { // To avoid printing duplicated error message to console | |||
log.New(os.Stderr, "", 0).Printf("ERROR: %v\n", s) |
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.
We'd better keep this check somewhere to avoid printing error message twice when logging to stderr.
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 can use log.Fatalf
to exit if some errors.
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.
log.Fatalf
will terminate the program, that's no good for the interactive mode of REPL.
pkg/sql/model.go
Outdated
@@ -86,7 +86,6 @@ func (m *model) saveDB(db *DB, table string, session *pb.Session) (e error) { | |||
if e != nil { | |||
return fmt.Errorf("cannot create sqlfs file %s: %v", table, e) | |||
} | |||
defer sqlf.Close() |
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.
How about defer
ing with the new Close
code in a lambda?
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 believe should check the error of hiveWriter.Close
, I updated this PR follow https://www.joeshaw.org/dont-defer-close-on-writable-files/ that uses a safe way to close sqlfs.
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.
Is it safe if we call hiveWriter.Close
twice?
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 checkout @shendiaomo 's comments and fix them in next PR?
fixed #1277
fixed #1317
related #1318