Kouhei Sutou
kou****@clear*****
Thu Aug 28 17:36:56 JST 2014
https://github.com/droonga/droonga-engine/commit/c9bad1eef0ce39eb7d412989b5802bfccaf62923 GitHubの方に書きましたが、できるだけクラスメッソドを定義する のは控えて欲しいです。。。まず、インスタンスメソッドだけじゃ だめなの？本当に？と検討したうえで、クラスメソッドを定義する ことを判断する、ぐらいの気持ちで書いて欲しいです。 class Parser class << self def parse parser = new parser.parse end end end くらいならよいと思いますが、 > + class NodeStatus > + class << self > + def have?(key) > + new.have?(key) > + end > + > + def get(key) > + new.get(key) > + end > + > + def set(key, value) > + new.set(key, value) > + end > + > + def delete(key) > + new.delete(key) > + end > + end はやり過ぎだと感じました。 if NodeStatus.have? NodeStatus.get end みたいにクラスをインスタンスのように使うことになってしまうか らです。 一般的にスコープが小さいほうが考えることが少なくなってよいで すが、クラスの方がインスタンスよりスコープが広いのでスコープ が大きくなりがちです。 例えば、このインスタンスはこのメソッドの中でしか使われていな い、みたいなことはすぐにわかりますが、クラスはどこからでも参 照できるので基本的に全部のファイルを確認しないといけません。 In <c9bad1eef0ce39eb7d412989b5802bfccaf62923 �� jenkins.clear-code.com> "[Groonga-commit] droonga/droonga-engine �� c9bad1e [master] Extract codes to manage status of the node" on Thu, 28 Aug 2014 16:40:31 +0900, YUKI Hiroshi <null+groonga �� clear-code.com> wrote: > YUKI Hiroshi 2014-08-28 16:40:31 +0900 (Thu, 28 Aug 2014) > > New Revision: c9bad1eef0ce39eb7d412989b5802bfccaf62923 > https://github.com/droonga/droonga-engine/commit/c9bad1eef0ce39eb7d412989b5802bfccaf62923 > > Message: > Extract codes to manage status of the node > > Added files: > lib/droonga/node_status.rb > Modified files: > lib/droonga/command/serf_event_handler.rb > lib/droonga/serf.rb > > Modified: lib/droonga/command/serf_event_handler.rb (+9 -22) > =================================================================== > --- lib/droonga/command/serf_event_handler.rb 2014-08-28 16:24:49 +0900 (ce42562) > +++ lib/droonga/command/serf_event_handler.rb 2014-08-28 16:40:31 +0900 (30afb73) > @@ -17,6 +17,7 @@ require "json" > > require "droonga/path" > require "droonga/serf" > +require "droonga/node_status" > require "droonga/catalog_generator" > require "droonga/catalog_modifier" > require "droonga/catalog_fetcher" > @@ -83,7 +84,7 @@ module Droonga > def process_event > case @event_sub_name > when "change_role" > - save_status(:role, @payload["role"]) > + NodeStatus.set(:role, @payload["role"]) > when "report_status" > report_status > when "join" > @@ -115,7 +116,7 @@ module Droonga > end > > def report_status > - @response["value"] = status(@payload["key"].to_sym) > + @response["value"] = NodeStatus.get(@payload["key"]) > end > > def join > @@ -177,13 +178,14 @@ module Droonga > end > sleep(5) #TODO: wait for restart. this should be done more safely, to avoid starting of absorbing with old catalog.json. > > - save_status(:absorbing, true) > + status = NodeStatus.new > + status.set(:absorbing, true) > DataAbsorber.absorb(:dataset => dataset_name, > :source_host => source_host, > :destination_host => host, > :port => port, > :tag => tag) > - delete_status(:absorbing) > + status.delete(:absorbing) > sleep(1) > end > > @@ -268,14 +270,15 @@ module Droonga > log("port = #{port}") > log("tag = #{tag}") > > - save_status(:absorbing, true) > + status = NodeStatus.new > + status.set(:absorbing, true) > DataAbsorber.absorb(:dataset => dataset_name, > :source_host => source, > :destination_host => host, > :port => port, > :tag => tag, > :client => "droonga-send") > - delete_status(:absorbing) > + status.delete(:absorbing) > end > > def live_nodes > @@ -289,22 +292,6 @@ module Droonga > SafeFileWriter.write(path, file_contents) > end > > - def status(key) > - Serf.status(key) > - end > - > - def save_status(key, value) > - status = Serf.load_status > - status[key] = value > - SafeFileWriter.write(Serf.status_file, JSON.pretty_generate(status)) > - end > - > - def delete_status(key) > - status = Serf.load_status > - status.delete(key) > - SafeFileWriter.write(Serf.status_file, JSON.pretty_generate(status)) > - end > - > def log(message) > @response["log"] << message > end > > Added: lib/droonga/node_status.rb (+85 -0) 100644 > =================================================================== > --- /dev/null > +++ lib/droonga/node_status.rb 2014-08-28 16:40:31 +0900 (0796098) > @@ -0,0 +1,85 @@ > +# Copyright (C) 2014 Droonga Project > +# > +# This library is free software; you can redistribute it and/or > +# modify it under the terms of the GNU Lesser General Public > +# License version 2.1 as published by the Free Software Foundation. > +# > +# This library is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > +# Lesser General Public License for more details. > +# > +# You should have received a copy of the GNU Lesser General Public > +# License along with this library; if not, write to the Free Software > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + > +require "json" > +require "droonga/path" > +require "droonga/safe_file_writer" > + > +module Droonga > + class NodeStatus > + class << self > + def have?(key) > + new.have?(key) > + end > + > + def get(key) > + new.get(key) > + end > + > + def set(key, value) > + new.set(key, value) > + end > + > + def delete(key) > + new.delete(key) > + end > + end > + > + def initialize > + @status = load > + end > + > + def have?(key) > + key = normalize_key(key) > + @status.include?(key) > + end > + > + def get(key) > + key = normalize_key(key) > + @status[key] > + end > + > + def set(key, value) > + key = normalize_key(key) > + @status[key] = value > + SafeFileWriter.write(status_file, JSON.pretty_generate(@status)) > + end > + > + def delete(key) > + key = normalize_key(key) > + @status.delete(key) > + SafeFileWriter.write(status_file, JSON.pretty_generate(@status)) > + end > + > + private > + def normalize_key(key) > + key.to_sym > + end > + > + def status_file > + @status_file ||= Path.state + "status_file" > + end > + > + def load > + if status_file.exist? > + contents = status_file.read > + unless contents.empty? > + return JSON.parse(contents, :symbolize_names => true) > + end > + end > + {} > + end > + end > +end > > Modified: lib/droonga/serf.rb (+6 -22) > =================================================================== > --- lib/droonga/serf.rb 2014-08-28 16:24:49 +0900 (387a68a) > +++ lib/droonga/serf.rb 2014-08-28 16:40:31 +0900 (44f4d03) > @@ -22,7 +22,9 @@ require "open3" > require "droonga/path" > require "droonga/loggable" > require "droonga/catalog_loader" > +require "droonga/node_status" > require "droonga/serf_downloader" > +require "droonga/safe_file_writer" > require "droonga/line_buffer" > > module Droonga > @@ -44,24 +46,6 @@ module Droonga > Droonga::Path.base + "serf" > end > > - def status_file > - Droonga::Path.state + "status_file" > - end > - > - def load_status > - if status_file.exist? > - contents = status_file.read > - unless contents.empty? > - return JSON.parse(contents, :symbolize_names => true) > - end > - end > - {} > - end > - > - def status(key) > - load_status[key] > - end > - > def send_query(name, query, payload) > new(nil, name).send_query(query, payload) > end > @@ -212,13 +196,13 @@ module Droonga > "#{extract_host(@name)}:7373" > end > > - def status > - @status ||= self.class.load_status > + def node_status > + @node_status ||= NodeStatus.new > end > > def role > - if status[:role] > - role = status[:role].to_sym > + if node_status.have?(:role) > + role = node_status.get(:role).to_sym > if self.class::ROLE.key?(role) > return role > end
