[
https://issues.apache.org/jira/browse/HDFS-7017?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14198855#comment-14198855
]
Haohui Mai commented on HDFS-7017:
----------------------------------
I'm with Colin in terms of {{std::bad_alloc}}. At this point I'm more concerned
about the correctness of the code. Taking care on {{std::bad_alloc}} seems a
pretty low priority to me, and it is still up to debate whether the exception
itself should be used.
Took a quick skim of the code. Some comments:
{code}
+
+class LeaseRenewer {
+public:
+ LeaseRenewer();
+ virtual ~LeaseRenewer();
+
+ virtual void StartRenew(shared_ptr<FileSystemImpl> filesystem) = 0;
+ virtual void StopRenew(shared_ptr<FileSystemImpl> filesystem) = 0;
+
+public:
+ static LeaseRenewer &GetLeaseRenewer();
+ static void CreateSingleton();
+
+private:
+ LeaseRenewer(const LeaseRenewer &other);
+ LeaseRenewer &operator=(const LeaseRenewer &other);
+
+ static once_flag once;
+ static shared_ptr<LeaseRenewer> renewer;
+};
+
{code}
It might be better to expose an {{instance()}} method directly in the class to
reflect the fact this is a singleton.
{code}
+
+LeaseRenewer::LeaseRenewer() {
+}
+
{code}
This is dead code.
{code}
+LeaseRenewerImpl::~LeaseRenewerImpl() {
+ stop = true;
+ cond.notify_all();
+
+ if (worker.joinable()) {
+ worker.join();
+ }
+}
+
{code}
It looks like the above code will never execute as the LeaseRenewerImpl never
get freed.
{code}
+class LeaseRenewerImpl : public LeaseRenewer {
+public:
+ LeaseRenewerImpl();
+ ~LeaseRenewerImpl();
+ int getInterval() const;
+ void setInterval(int interval);
+ void StartRenew(shared_ptr<FileSystemImpl> filesystem);
+ void StopRenew(shared_ptr<FileSystemImpl> filesystem);
+
+private:
+ void renewer();
+
+private:
+ LeaseRenewerImpl(const LeaseRenewerImpl &other);
+ LeaseRenewerImpl &operator=(const LeaseRenewerImpl &other);
+
+ atomic<bool> stop;
+ condition_variable cond;
+ int interval;
+ mutex mut;
+ std::map<std::string, shared_ptr<FileSystemImpl>> maps;
+ thread worker;
+};
+}
{code}
Since {{LeaseRenewer}} is a private class / interface, it works better to
combine {{LeaseRenewer}} and {{LeaseRenewerImpl}}
{code}
+void OutputStreamImpl::append(const char *buf, int64_t size) {
{code}
should {{size}} be unsigned? What is maximum value of the size?
{code}
+void OutputStreamImpl::completeFile(bool throwError) {
{code}
You can return a {{Status}} object and let the caller to decide whether to
throw the exception.
{code}
+shared_ptr<Packet> PacketPool::getPacket(int pktSize, int chunksPerPkt,
+ int64_t offsetInBlock, int64_t seqno,
+ int checksumSize) {
+ if (packets.empty()) {
+ return shared_ptr<Packet>(new Packet(
+ pktSize, chunksPerPkt, offsetInBlock, seqno, checksumSize));
+ } else {
+ shared_ptr<Packet> retval = packets.front();
+ packets.pop_front();
+ retval->reset(pktSize, chunksPerPkt, offsetInBlock, seqno,
+ checksumSize);
+ return retval;
+ }
+}
{code}
The pool might need to block to guard against overcommit (it can be addressed
in a separate jira). And to really avoid the cost of allocation, the pool needs
to be backed by untyped arenas. I suggest removing it for now to simplify the
code.
> Implement OutputStream for libhdfs3
> -----------------------------------
>
> Key: HDFS-7017
> URL: https://issues.apache.org/jira/browse/HDFS-7017
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: Zhanwei Wang
> Assignee: Zhanwei Wang
> Attachments: HDFS-7017-pnative.002.patch,
> HDFS-7017-pnative.003.patch, HDFS-7017-pnative.004.patch, HDFS-7017.patch
>
>
> Implement pipeline and OutputStream C++ interface
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
