bakaid commented on a change in pull request #558: MINIFICPP-542 - Add PutSFTP processor URL: https://github.com/apache/nifi-minifi-cpp/pull/558#discussion_r285663616
########## File path: extensions/sftp/client/SFTPClient.h ########## @@ -0,0 +1,183 @@ +/** + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef __SFTP_CLIENT_H__ +#define __SFTP_CLIENT_H__ + +#include <curl/curl.h> +#include <libssh2.h> +#include <libssh2_sftp.h> +#include <vector> +#include <iostream> +#include <string> +#include <uuid/uuid.h> +#include <vector> + +#include "utils/HTTPClient.h" +#include "core/logging/Logger.h" +#include "core/logging/LoggerConfiguration.h" +#include "properties/Configure.h" +#include "io/validation.h" +#include "io/BaseStream.h" + +namespace org { +namespace apache { +namespace nifi { +namespace minifi { +namespace utils { + +/** + * Initializes and cleans up curl and libssh2 once. + * Cleanup will only occur at the end of our execution since we are relying on a static variable. + */ +class SFTPClientInitializer { + public: + static SFTPClientInitializer *getInstance() { + static SFTPClientInitializer initializer; + return &initializer; + } + void initialize() { + + } + private: + SFTPClientInitializer() { + libssh2_init(0); + curl_global_init(CURL_GLOBAL_DEFAULT); Review comment: @phrocker On grouping the extensions by dependencies: from our standpoint that could make sense (it could solve our current problem, for example), as long as extensions do not depend on different sets of multiple external dependencies. The sftp extension depends on curl and libssh2, so we could group it under extensions depending on curl, with an extra attribute of depending on libssh2. But what would happen if in the future we want to create an extension which depends on libssh2 but does not depend on curl? (I don’t think this particular case will happen, but I’m trying to figure out a general architecture.) In that case we will still have to group it under curl-dependent extensions and make it dependent on curl, as we could not create a different initializer for it that calls libssh2_init, because that could potentially be called in parallel with HTTPCurlLoader. Even worse, If our extension would depend on libssh2 and libfoobar, we would have to group all extensions depending on libfoobar with the extensions depending both on curl and libssh2, as we couldn’t allow having two initializers calling libfoobar_init. From the standpoint of the user I would imagine (but I could be terribly wrong in this case), that they do not care about the thirdparty dependencies we use to implement processors - they care about the particular processor and the functionality it brings. They don’t want to have all functionality that could be implemented with curl - they want SFTP, FTP or HTTP. So to recap: grouping similar processors into the same extensions, especially if they have the same dependencies could make sense on a case-to-case basis but I don’t think that grouping all processors depending on a particular external dependency into the same extension solves our problems or brings much to the table (we could solve running out of letters in bootstrap.sh by other means), but brings its own set of problems. A way to solve this based on your recommendation perhaps would be to eagerly initialize the components in the loader AND call these initialization functions under a global lock (move the lock in ClassLoader::registerResource up a few lines). This way we wouldn’t have to change the architecture drastically, but still wouldn’t have to group all processors depending on curl (and all other transitive dependencies) in the same extension. I too will think about this more. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
