)]}'
{"Engine/swift/storlet_gateway/common/file_manager.py":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"d78bca63cb87678bd699abb910dbaf372c3977df","unresolved":false,"context_lines":[{"line_number":14,"context_line":"# limitations under the License."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"class FileManager(object):"},{"line_number":18,"context_line":"    \"\"\""},{"line_number":19,"context_line":"    This class is used to load/save files required for storlet execution"},{"line_number":20,"context_line":"    from/into the storage which also stores data to be processed"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_e6521b12","line":17,"updated":"2016-08-02 01:34:37.000000000","message":"IMHO, this class is meaningless because we have only one child class of this and probably we don\u0027t have the future plan to implement any others for now, right? \n\nExactly, someone wants to make another in the future but currently this one is only working to confuse to track \"what\u0027s general thing but nothing implemented as general\".\nIt makes the code readability worse, I believe.\n\nPlus, since the design, this should belongs to storlet_middleware components as well as the child?\n\nI don\u0027t like to go back and forth the components while reading the code, unnecessarily.","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"0f4f8fbd28204609063b3d5f1b6b8b3cf1437982","unresolved":false,"context_lines":[{"line_number":14,"context_line":"# limitations under the License."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"class FileManager(object):"},{"line_number":18,"context_line":"    \"\"\""},{"line_number":19,"context_line":"    This class is used to load/save files required for storlet execution"},{"line_number":20,"context_line":"    from/into the storage which also stores data to be processed"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_7749b330","line":17,"in_reply_to":"bacf61ea_e6521b12","updated":"2016-08-02 04:06:14.000000000","message":"\u003e IMHO, this class is meaningless because we have only one child class of this and probably we don\u0027t have the future plan to implement any others for now, right?\n\nright.\nThis interface definition is not necesorry if we don\u0027t have any plan to create new gateways or add support for new storage. \n\n\u003e Exactly, someone wants to make another in the future but currently this one is only working to confuse to track \"what\u0027s general thing but nothing implemented as general\".\nIt makes the code readability worse, I believe.\n\u003e \n\u003e Plus, since the design, this should belongs to storlet_middleware components as well as the child?\n\nI don\u0027t agree.\nHaving the following three things is general for all storages and all storage gateways.\n\n * storlet file (application file)\n * dependency file (some files required to execute an application)\n * storlet execution log\n\nIt does not depends on swift, and is generally necessory when we execute some an application, so should be defined as a general interface.\nThat is the reason why I created this interface class in common place.","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"}],"Engine/swift/storlet_gateway/gateways/docker/gateway.py":[{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"d78bca63cb87678bd699abb910dbaf372c3977df","unresolved":false,"context_lines":[{"line_number":67,"context_line":"                             if x.strip()]"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        # TODO(takashi): file manager should not be an optional parameter,"},{"line_number":70,"context_line":"        #                but a required parameter"},{"line_number":71,"context_line":"        self.file_manager \u003d self.options.get(\u0027file_manager\u0027)"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"        self.start \u003d self.options.get(\u0027range_start\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_c3fb8945","line":70,"updated":"2016-08-02 01:34:37.000000000","message":"Yes, that *MUST* be. I think it should be in *BEFORE* landing. Otherwise, it can make a bunch of bugs easily.","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"0f4f8fbd28204609063b3d5f1b6b8b3cf1437982","unresolved":false,"context_lines":[{"line_number":67,"context_line":"                             if x.strip()]"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"        # TODO(takashi): file manager should not be an optional parameter,"},{"line_number":70,"context_line":"        #                but a required parameter"},{"line_number":71,"context_line":"        self.file_manager \u003d self.options.get(\u0027file_manager\u0027)"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"        self.start \u003d self.options.get(\u0027range_start\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_17ecb728","line":70,"in_reply_to":"bacf61ea_c3fb8945","updated":"2016-08-02 04:06:14.000000000","message":"I agree with this, sorry.\n\nPlease find the follow-ip patch about this point. [1]\n# I still need to fix gete job failure...\n\n[1] https://review.openstack.org/#/c/349566/","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"d78bca63cb87678bd699abb910dbaf372c3977df","unresolved":false,"context_lines":[{"line_number":179,"context_line":"        \"\"\""},{"line_number":180,"context_line":"        # Determine the cache we are to work with"},{"line_number":181,"context_line":"        # e.g. dependency or storlet"},{"line_number":182,"context_line":"        if is_storlet:"},{"line_number":183,"context_line":"            cache_dir \u003d self.paths.get_host_storlet_cache_dir()"},{"line_number":184,"context_line":"            get_func \u003d sreq.file_manager.get_storlet"},{"line_number":185,"context_line":"        else:"},{"line_number":186,"context_line":"            cache_dir \u003d self.paths.get_host_dependency_cache_dir()"},{"line_number":187,"context_line":"            get_func \u003d sreq.file_manager.get_dependency"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":"        if not os.path.exists(cache_dir):"},{"line_number":190,"context_line":"            os.makedirs(cache_dir, 0o755)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_c380a98d","line":187,"range":{"start_line":182,"start_character":0,"end_line":187,"end_character":55},"updated":"2016-08-02 01:34:37.000000000","message":"I think these lines are evidence that this refactoring is not enough because it cannot encapsulate those method.\n\nIMO, we shouldn\u0027t assign the instance method for the local variable as possible because it blinds us to know what method actually called. If we want to encapsulate them and really need the transparent access for both of them, the handling should be in file_manager.","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"0f4f8fbd28204609063b3d5f1b6b8b3cf1437982","unresolved":false,"context_lines":[{"line_number":179,"context_line":"        \"\"\""},{"line_number":180,"context_line":"        # Determine the cache we are to work with"},{"line_number":181,"context_line":"        # e.g. dependency or storlet"},{"line_number":182,"context_line":"        if is_storlet:"},{"line_number":183,"context_line":"            cache_dir \u003d self.paths.get_host_storlet_cache_dir()"},{"line_number":184,"context_line":"            get_func \u003d sreq.file_manager.get_storlet"},{"line_number":185,"context_line":"        else:"},{"line_number":186,"context_line":"            cache_dir \u003d self.paths.get_host_dependency_cache_dir()"},{"line_number":187,"context_line":"            get_func \u003d sreq.file_manager.get_dependency"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":"        if not os.path.exists(cache_dir):"},{"line_number":190,"context_line":"            os.makedirs(cache_dir, 0o755)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_77a1b393","line":187,"range":{"start_line":182,"start_character":0,"end_line":187,"end_character":55},"in_reply_to":"bacf61ea_c380a98d","updated":"2016-08-02 04:06:14.000000000","message":"agree, and need to refactor some more.\n\nIMO it is not a very good way to pass boolean value to decide if the file is storlet or depency, so we need some improvoments about the code about cache management.","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"},{"author":{"_account_id":4608,"name":"Kota Tsuyuzaki","email":"bloodeagle40234@gmail.com","username":"tsuyuzaki-kota"},"change_message_id":"d78bca63cb87678bd699abb910dbaf372c3977df","unresolved":false,"context_lines":[{"line_number":217,"context_line":"        if update_cache:"},{"line_number":218,"context_line":"            # If the cache needs to be updated, then get on with it"},{"line_number":219,"context_line":"            # bring the object from storge"},{"line_number":220,"context_line":"            data_iter, perm \u003d get_func(obj_name)"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"            # TODO(takashi): Do not directly write to target path"},{"line_number":223,"context_line":"            with open(cache_target_path, \u0027w\u0027) as fn:"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_e3368d01","line":220,"updated":"2016-08-02 01:34:37.000000000","message":"Here, get_func means nothing that makes developer complicated. If we want to generalize, I think what we need is likely:\n\n with file_manager.get_file() as f:\n    body \u003d f.read()\n\n?","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"0f4f8fbd28204609063b3d5f1b6b8b3cf1437982","unresolved":false,"context_lines":[{"line_number":217,"context_line":"        if update_cache:"},{"line_number":218,"context_line":"            # If the cache needs to be updated, then get on with it"},{"line_number":219,"context_line":"            # bring the object from storge"},{"line_number":220,"context_line":"            data_iter, perm \u003d get_func(obj_name)"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"            # TODO(takashi): Do not directly write to target path"},{"line_number":223,"context_line":"            with open(cache_target_path, \u0027w\u0027) as fn:"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_b7fdebb2","line":220,"in_reply_to":"bacf61ea_e3368d01","updated":"2016-08-02 04:06:14.000000000","message":"The reason why I used a tuple as a return value for get_func is that we need to get the following two things from storage.\n\n * File content\n * Permission\n\nMy first idea was having two functions get_file and get_permission, but it requires doubled requests to swift.\n\nIn my current thought, we need to wrap iterator with FileLikeIter, and add some instance value to store permission parameter in that instance.","commit_id":"26a12ec5c1e1c931156dcb8952cb02578970e0c6"}]}