szaszm commented on code in PR #1328:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1328#discussion_r929052167
##########
libminifi/include/core/repository/VolatileContentRepository.h:
##########
@@ -68,55 +59,55 @@ class VolatileContentRepository :
}
master_list_.clear();
}
+ stop();
}
/**
* Initialize the volatile content repo
* @param configure configuration
*/
- virtual bool initialize(const std::shared_ptr<Configure> &configure);
-
- /**
- * Stop any thread associated with the volatile content repository.
- */
- virtual void stop();
+ bool initialize(const std::shared_ptr<Configure> &configure) override;
/**
* Creates writable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream the consumer
will write to.
*/
- virtual std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim
&claim, bool append);
+ std::shared_ptr<io::BaseStream> write(const minifi::ResourceClaim &claim,
bool append) override;
/**
* Creates readable stream.
* @param claim resource claim
* @return BaseStream shared pointer that represents the stream from which
the consumer will read..
*/
- virtual std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim
&claim);
+ std::shared_ptr<io::BaseStream> read(const minifi::ResourceClaim &claim)
override;
- virtual bool exists(const minifi::ResourceClaim &streamId);
+ bool exists(const minifi::ResourceClaim &streamId) override;
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in
volatile memory.
*/
- virtual bool close(const minifi::ResourceClaim &claim) {
+ bool close(const minifi::ResourceClaim &claim) override {
return remove(claim);
}
/**
* Closes the claim.
* @return whether or not the claim is associated with content stored in
volatile memory.
*/
- virtual bool remove(const minifi::ResourceClaim &claim);
+ bool remove(const minifi::ResourceClaim &claim) override;
+
+ private:
+ void run() override {
+ }
- protected:
- virtual void start();
+ std::thread& getThread() override {
+ return thread_;
+ }
Review Comment:
You're right, I asked for it, because I thought that all repositories are
"threaded". Obviously that's not the case. Additionally, I don't think
`ThreadedRepository`, or the "threadedness" of a repository is something user
code should be concerned with, so I don't say we should go back to the previous
revision. But an empty/noop implementation is basically a violation of the
[LSP](https://en.wikipedia.org/wiki/Liskov_substitution_principle).
My first thought would be to add a thread to every "threaded" repostitory,
and implement the start/stop logic in all such leaf classes. That would go
against [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself). That
could be solved by reintroducing `ThreadedRepository` as an implementation
helper, **not exposed to user code**. This is still an anti-pattern, by reusing
implementation through inheritance, but this seems to be the least evil, and if
we can enforce no naming of this class outside of Repository implementations,
then it's relatively contained and harmless IMO. I would do this. If you know
better, please comment it, so we can discuss.
In general, you understand your code best. When I comment an idea, and you
run into an issue with it (e.g.
[LSP](https://en.wikipedia.org/wiki/Liskov_substitution_principle) violation in
the last case), or I just comment something stupid, please ask or push back.
Solving a small problem by making a bigger one is not good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
