Introduce Multi Cluster Management #285
Conversation
lib/algolia/client.rb
Outdated
| delete(Protocol.cluster_mapping_uri, :write, request_options) | ||
| end | ||
|
|
||
| def search_user_id(query, clusterName = nil, page = nil, hits_per_page = nil, request_options = {}) |
lib/algolia/client.rb
Outdated
| end | ||
|
|
||
| def search_user_id(query, clusterName = nil, page = nil, hits_per_page = nil, request_options = {}) | ||
| body = {:query => query} |
There was a problem hiding this comment.
{:query => query} => { :query => query }
There was a problem hiding this comment.
BTW I didn't mark all of them, just one example
lib/algolia/protocol.rb
Outdated
|
|
||
| def Protocol.cluster_mapping_uri(user_id = nil) | ||
| if user_id | ||
| user_id = "/#{CGI.escape(user_id)}" |
There was a problem hiding this comment.
Usually inlined if are written after the statement in Ruby: user_id = "/#{CGI.escape(user_id)}" if user_id
lib/algolia/protocol.rb
Outdated
| if user_id | ||
| user_id = "/#{CGI.escape(user_id)}" | ||
| end | ||
| "/#{VERSION}/clusters/mapping" + user_id.to_s |
There was a problem hiding this comment.
Is this endpoint valid /#{VERSION}/clusters/mapping if user_id is nil?
There was a problem hiding this comment.
Yes, it's use to add/remove user IDs, I didn't want to create 2 entries in Protocol but maybe that would be clearer.
spec/client_mcm_spec.rb
Outdated
| end | ||
|
|
||
| after(:all) do | ||
| # @index.delete_index rescue "not fatal" |
|
|
||
| it "should remove a user_id" do | ||
| res = @client.remove_user_id(@user_id) | ||
| sleep(2) |
There was a problem hiding this comment.
There is no waitTask for MCM?
There was a problem hiding this comment.
No :(
A better way to do this would be to loop and call multiple times to see if it was removed. After X attemps, the test would fail.
lib/algolia/client.rb
Outdated
| end | ||
|
|
||
| def assign_user_id(user_id, cluster_name, request_options = {}) | ||
| if !request_options["headers"].is_a?(Hash) |
There was a problem hiding this comment.
You can use single quotes when there is no interpolation to perform
spec/spec_helper.rb
Outdated
| @@ -26,6 +26,7 @@ | |||
|
|
|||
| RSpec.configure do |config| | |||
| config.mock_with :rspec | |||
| config.expect_with(:rspec) { |c| c.syntax = :should } | |||
There was a problem hiding this comment.
Oh my bad, since it is currently working with expect for the oldest Ruby version already, I think it's not necessary to downgrade the syntax
There was a problem hiding this comment.
I understood this opposite. I thought expect was the new way and we were using include for older version. I had a warning saying it should be activated explicitely. I'll check again 👌
There was a problem hiding this comment.
You are right, it's the new way but it is working for old rubies; Take a look at the latest Travis run, all green even on 1.8.xx
lib/algolia/client.rb
Outdated
| end | ||
|
|
||
| def remove_user_id(user_id, request_options = {}) | ||
| if !request_options["headers"].is_a?(Hash) |
|
@raphi Let me know what you think of the 3rd commit. |
|
Updated! Thanks for review |
|
@julienbourdeau np! Please just revert |
julienbourdeau
force-pushed
the
feat/mcm
branch
2 times, most recently
from
d117a9a
to
bda8214
Compare
|
Build is greed ✅ |
|
@julienbourdeau what did you changed though? It seems there might be some concurrency issues in the MCM tests |
|
I think when they run all at once, the delay might be a bit longer to assign/remove user ids but the name shouldn't collide. |
|
I don't remember the error exactly but on top of my head it was something like "MCM already processing id whatever" so probably a delay issue. |
|
Yes, I think it was occuring when trying to remove a user id while the assigning task wasn't finished. There is no taskID for this type of job :( |
Related PR:
Doc snippets are coming