-
Notifications
You must be signed in to change notification settings - Fork 59
feat(bulk_load): add start bulk load http interface #693
Conversation
@@ -6,11 +6,15 @@ | |||
|
|||
#include <algorithm> | |||
|
|||
#include <dsn/cpp/json_helper.h> |
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.
you can move this line to the corresponding cpp file
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.
This update is for further pull request, I will add structure in .h
file using json serilization.
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.
So I think you can add this line in the future pull request, because it's meaningless for this pr.
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.
It is still useful for this pull request, I use NON_MEMBER_JSON_SERIALIZATION
marco to make structure start_bulk_load_request
json-serializable. As your comment shows, I can add this in meta_http_service.cpp
, but considering it will finally move into .h
file, I did it in this pull request.
resp.body = "remote_root_path should not be empty"; | ||
resp.status_code = http_status_code::bad_request; | ||
return; | ||
} |
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.
Can this simple args validate put client side?
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.
In current implmetation, client and server both will validate arguments.
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 will implement HTTP argument validation in later PR. I actually did it in my staging branch. But haven't ready for review yet. Got no time to make it steady.
resp.body = "remote_root_path should not be empty"; | ||
resp.status_code = http_status_code::bad_request; | ||
return; | ||
} |
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 will implement HTTP argument validation in later PR. I actually did it in my staging branch. But haven't ready for review yet. Got no time to make it steady.
This pull request adds a new http interface for meta server to start bulk load.