-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Add client interface. #16
Changes from 2 commits
6ffea6e
94af15b
f69523a
ccf6cc8
5e26f58
020093c
ee222f0
1b0ccac
b3a2e1e
ec8f500
5dc12cb
0d59c62
4cd6233
fec5e73
a5e799b
1b15b5d
98104f1
cb369fd
b4a6f36
f3de2ca
70547ae
525311c
c776376
49c571e
43d7e16
5e8e1dd
fe23c3c
064edd8
e7c5240
b0b414e
97761e1
91d36f2
84c2f41
f7ab013
302e68a
a2dc8bc
046e740
92d6489
d27f042
2b49746
c4ee015
0cc231b
069a7a7
9069030
0fe063f
e089107
159aa08
8e035b7
9d90c37
8de26ec
b9bfcdd
351c8b5
7dae8d4
5d15b22
1588b51
1e1e41d
c0a6e6f
99c5935
80f314a
7796e15
036dd51
8affa10
77d2458
026b6f1
448f693
769a708
3555d2e
a0c5b3a
e185bf3
f0d79e9
f444772
530da78
db21fe7
165eb84
06de774
c7a07b1
f8c623a
488eaa3
f0729d9
4d2aa6c
88ed638
bd55552
9897995
73eabef
85d2475
61be939
5bffeee
22f370d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
#include "Data.hpp" | ||
|
||
#include <functional> | ||
#include <string> | ||
#include <vector> | ||
|
||
namespace spider { | ||
|
||
class DataImpl {}; | ||
|
||
template <class T> | ||
auto Data<T>::get() -> T { | ||
return T(); | ||
} | ||
|
||
template <class T> | ||
void Data<T>::set_locality(std::vector<std::string> const& /*nodes*/, bool /*hard*/) {} | ||
|
||
template <class T> | ||
auto Data<T>::Builder::key(std::string const& /*key*/) -> Data<T>::Builder& { | ||
return this; | ||
} | ||
|
||
template <class T> | ||
auto Data<T>::Builder::locality(std::vector<std::string> const& /*nodes*/, bool /*hard*/) | ||
-> Data<T>::Builder& { | ||
return this; | ||
} | ||
|
||
template <class T> | ||
auto Data<T>::Builder::cleanup(std::function<T const&()> const& /*f*/) -> Data<T>::Builder& { | ||
return this; | ||
} | ||
|
||
template <class T> | ||
auto Data<T>::Builder::build(T const& /*t*/) -> Data<T> { | ||
return Data<T>(); | ||
} | ||
|
||
} // namespace spider |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,81 @@ | ||||||||||||
#ifndef SPIDER_CLIENT_DATA_HPP | ||||||||||||
#define SPIDER_CLIENT_DATA_HPP | ||||||||||||
|
||||||||||||
#include <functional> | ||||||||||||
#include <memory> | ||||||||||||
#include <string> | ||||||||||||
#include <vector> | ||||||||||||
|
||||||||||||
namespace spider { | ||||||||||||
|
||||||||||||
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
class DataImpl; | ||||||||||||
|
||||||||||||
template <class T> | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like I mentioned in Spider.hpp, we can restrict the types using C++20 concepts. |
||||||||||||
class Data { | ||||||||||||
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
private: | ||||||||||||
std::unique_ptr<DataImpl> m_impl; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Private declarations should come after public. |
||||||||||||
|
||||||||||||
public: | ||||||||||||
/** | ||||||||||||
* Gets the values stored in Data. | ||||||||||||
* @return value stored in Data. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
As our internal guidelines mention, for most getters, we can omit the docstring description and only describe the return value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the special case. This is not an ordinary getter. It accesses data storage under the hood and could throw exception, or return error once we get error included in the interface. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since neither the exception or error are currently in the code, let's write the docstring according to what exists (not what will exist later). When we eventually add the error information, we can update the docstring appropriately. |
||||||||||||
*/ | ||||||||||||
auto get() -> T; | ||||||||||||
/** | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Add an empty line between methods. |
||||||||||||
* Indicates that the data is persisted and should not be rollbacked | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||
* on failure recovery. | ||||||||||||
*/ | ||||||||||||
// Not implemented in milestone 1 | ||||||||||||
// void mark_persist(); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my experience, it's a bad idea to add code that we'll use in the future since that day may never come. If that day comes, we will remember what to do based on the design doc rather than this commented code. |
||||||||||||
/** | ||||||||||||
* Sets locality list of the data. | ||||||||||||
* @param nodes nodes that has locality | ||||||||||||
* @param hard true if the locality list is a hard requirement, false otherwise | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These docstrings are a bit unclear to me. I guess the idea is that if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Hard requirement means data can only be accessed on nodes in the locality list. Will change the docstring to make it clear. |
||||||||||||
*/ | ||||||||||||
void set_locality(std::vector<std::string> const& nodes, bool hard); | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Locality information might not be available at the time we create the data. For example, we create an associated data for HDFS files before we create the actual HDFS files and write to them so that Spider can gc the HDFS files on failure during write. However, we don't know the locality of the HDFS files before creating them, and thus we need a way to set locality on the fly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||
|
||||||||||||
class Builder { | ||||||||||||
private: | ||||||||||||
public: | ||||||||||||
/** | ||||||||||||
* Sets the key for the data. If no key is provided, Spider generates a key. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the key is optional, how about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry. Wrong docstring. Spider doesn't generate a key if no key is provided. |
||||||||||||
* @param key of the data | ||||||||||||
*/ | ||||||||||||
auto key(std::string const& key) -> Data<T>::Builder&; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Return value is missing from docstring. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be clearer to name all these setters as |
||||||||||||
/** | ||||||||||||
* Sets locality list of the data to build. | ||||||||||||
* @param nodes nodes that has locality | ||||||||||||
* @param hard true if the locality list is a hard requirement, false otherwise | ||||||||||||
* @return self | ||||||||||||
*/ | ||||||||||||
auto locality(std::vector<std::string> const& nodes, bool hard) -> Data<T>::Builder&; | ||||||||||||
/** | ||||||||||||
* Indicates that the data to build is persisted and should not be rollbacked on failure | ||||||||||||
* recovery. | ||||||||||||
* @return self | ||||||||||||
*/ | ||||||||||||
// Data<T>::Builder Builder& mark_persist(); // Not implemented in milestone 1 | ||||||||||||
/** | ||||||||||||
* Defines clean up functions of the data to build. | ||||||||||||
* @param f clean up function of data | ||||||||||||
*/ | ||||||||||||
auto cleanup(std::function<T const&()> const& f) -> Data<T>::Builder&; | ||||||||||||
/** | ||||||||||||
* Defines rollback functions of the data to build. | ||||||||||||
* @param f rollback function of data | ||||||||||||
*/ | ||||||||||||
// Not implemented for milestone 1 | ||||||||||||
// auto rollback(std::function<const T&()> const& f) -> Data<T>::Builder&; | ||||||||||||
/** | ||||||||||||
* Builds the data. Stores the value of data into storage with locality list, persisted | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this class store the data? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Internally all values are serialized and stored in data storage. |
||||||||||||
* flag, cleanup and rollback functions. | ||||||||||||
* @param t value of the data | ||||||||||||
* @return data object | ||||||||||||
*/ | ||||||||||||
auto build(T const& /*t*/) -> Data<T>; | ||||||||||||
}; | ||||||||||||
}; | ||||||||||||
|
||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
} // namespace spider | ||||||||||||
|
||||||||||||
#endif // SPIDER_CLIENT_DATA_HPP |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
#include "Future.hpp" | ||
|
||
#include <boost/uuid/uuid.hpp> | ||
#include <boost/uuid/uuid_io.hpp> | ||
#include <string> | ||
|
||
namespace spider { | ||
|
||
class FutureImpl { | ||
// Implementation details subject to change | ||
private: | ||
boost::uuids::uuid m_id; | ||
|
||
public: | ||
auto value() -> std::string { return boost::uuids::to_string(m_id); } | ||
|
||
auto ready() -> bool { return m_id.is_nil(); } | ||
}; | ||
|
||
template <class T> | ||
auto Future<T>::get() -> T { | ||
return T(); | ||
} | ||
|
||
template <class T> | ||
auto Future<T>::ready() -> bool { | ||
return true; | ||
} | ||
|
||
} // namespace spider |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
#ifndef SPIDER_CLIENT_FUTURE_HPP | ||
#define SPIDER_CLIENT_FUTURE_HPP | ||
|
||
#include <memory> | ||
|
||
namespace spider { | ||
|
||
class FutureImpl; | ||
|
||
template <class T> | ||
class Future { | ||
private: | ||
std::unique_ptr<FutureImpl> m_impl; | ||
|
||
public: | ||
/** | ||
* Gets the value of the future. Blocks until the value is available. | ||
* @return value of the future | ||
*/ | ||
auto get() -> T; | ||
|
||
/** | ||
* Checks if value of the future is ready. | ||
* @return true if future is ready, false otherwise | ||
*/ | ||
auto ready() -> bool; | ||
}; | ||
|
||
} // namespace spider | ||
|
||
#endif // SPIDER_CLIENT_FUTURE_HPP |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this file doesn't correspond to a class, the filename should be lowercase. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
#ifndef SPIDER_CLIENT_SPIDER_HPP | ||
#define SPIDER_CLIENT_SPIDER_HPP | ||
|
||
#include <functional> | ||
#include <string> | ||
|
||
// NOLINTBEGIN(misc-include-cleaner) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be using IWYU's pragmas, right? |
||
#include "Data.hpp" | ||
#include "Future.hpp" | ||
#include "Task.hpp" | ||
#include "TaskGraph.hpp" | ||
|
||
// NOLINTEND(misc-include-cleaner) | ||
|
||
namespace spider { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading the functions in this file and those in |
||
/** | ||
* Initializes Spider library | ||
*/ | ||
void init(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What will this function do internally? |
||
|
||
/** | ||
* Connects to storage | ||
* @param url url of the storage to connect | ||
*/ | ||
void connect(std::string const& url); | ||
|
||
/** | ||
* Registers function to Spider | ||
* @param function function to register | ||
*/ | ||
template <class R, class... Args> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's replace On that note though, if the requirement is that the value types need to be serializable, why don't we define an interface for serializable value types and then the user has the flexibility to use any serializable type they want? We could do this later, but conceptually, is it possible? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also use the same C++20 concepts wherever we have task inputs/outputs. This would also simplify the docstrings since we don't need to repeat what types are acceptable. |
||
void register_task(std::function<R(Args...)> const& function); | ||
|
||
/** | ||
* Registers function to Spider with timeout | ||
* @param function_name name of the function to register | ||
* @param timeout task is considered straggler after timeout ms, and Spider triggers replicate the | ||
* task | ||
*/ | ||
template <class R, class... Args> | ||
void register_task(std::function<R(Args...)> const& function, float timeout); | ||
|
||
} // namespace spider | ||
|
||
#endif // SPIDER_CLIENT_SPIDER_HPP |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
#include "Task.hpp" | ||
|
||
#include <optional> | ||
#include <string> | ||
|
||
#include "Data.hpp" | ||
|
||
namespace spider { | ||
|
||
template <typename T> | ||
auto get_data(std::string const& /*key*/) -> std::optional<Data<T>> { | ||
return std::nullopt; | ||
} | ||
|
||
} // namespace spider |
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 should document template parameters as well.