ctstone / csredis

.NET client for Redis and Redis Sentinel (2.8). Includes both synchronous and asynchronous clients.
Other
292 stars 111 forks source link

HashMapper.ToObject for Sentinel Nodes #6

Closed dbartokthomas closed 10 years ago

dbartokthomas commented 10 years ago

When trying to get a master/slave, the library is issuing the "SENTINEL slaves mymaster" command. Which is returning...

1) "name" 2) "192.168.107.121:6401" 3) "ip" 4) "192.168.107.121" 5) "port" 6) "6401" 7) "runid" 8) "caadce2a27f1d705dc294f954e9fefd0afb20a40" 9) "flags" 10) "slave" 11) "pending-commands" 12) "0" 13) "last-ping-sent" 14) "0" 15) "last-ok-ping-reply" 16) "515" 17) "last-ping-reply" 18) "515" 19) "down-after-milliseconds" 20) "2000" 21) "info-refresh" 22) "4572" 23) "role-reported" 24) "slave" 25) "role-reported-time" 26) "4930667" 27) "master-link-down-time" 28) "0" 29) "master-link-status" 30) "ok" 31) "master-host"

and many many more, currently it's failing to map those back to the objects using the code in ToObject.

            if (fieldValues.Length == 0)
                return default(T);
            var dict = new Dictionary<string, string>();
            for (int i = 0; i < fieldValues.Length; i += 2)
            {
                string key = fieldValues[i].ToString().Replace("-", String.Empty);
                if (key == "name")
                    key = "Name";
                string value = fieldValues[i + 1].ToString();
                dict[key] = value;
            }
            return Serializer<T>.Deserialize(dict);

Above this there are a few lines that are commented out

            T obj = Activator.CreateInstance<T>();

            TypeConverter conv;
            PropertyInfo prop;
            string field, value;
            for (int i = 0; i < fieldValues.Length; i += 2)
            {
                field = fieldValues[i].ToString().Replace("-", String.Empty);
                value = fieldValues[i + 1].ToString();
                prop = typeof(T).GetProperty(field, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance);
                if (prop == null || !prop.CanWrite)
                    continue;
                conv = TypeDescriptor.GetConverter(prop.PropertyType);
                if (!conv.CanConvertFrom(typeof(String)))
                    continue;
                if (prop.PropertyType == typeof(Boolean) && value == "1")
                    value = "true";
                prop.SetValue(obj, conv.ConvertFrom(value), null);
            }
            return obj;

All of which work a treat. I've swapped out the code again and it works again now. This probably isn't your fix, just bringing it to your attention.

ctstone commented 10 years ago

Turns out this is a simple case-sensitivity bug. The .NET class is camel cased, but the Redis reply is lowercase.

Since the failure is in a dictionary TryGetValue call, the simplest fix is probably to just refactor the Sentinel models to match the server reply. I'll try a few other ideas first to see if there is a better way to handle this.

That said: I suspect the sentinel responses have undergone some changes since this package was written (around 2.6). Since csredis models are likely incomplete/wrong, I recommend using the lower level sentinel.Call("SENTINEL slaves", "my master") to completely bypass serialization for now. Or at least until I can get some proper test cases around Sentinel.

On Mon, Apr 7, 2014 at 9:31 AM, Dean Thomas notifications@github.comwrote:

When trying to get a master/slave, the library is issuing the "SENTINEL slaves mymaster" command. Which is returning...

1) "name" 2) "192.168.107.121:6401" 3) "ip" 4) "192.168.107.121" 5) "port" 6) "6401" 7) "runid" 8) "caadce2a27f1d705dc294f954e9fefd0afb20a40" 9) "flags" 10) "slave" 11) "pending-commands" 12) "0" 13) "last-ping-sent" 14) "0" 15) "last-ok-ping-reply" 16) "515" 17) "last-ping-reply" 18) "515" 19) "down-after-milliseconds" 20) "2000" 21) "info-refresh" 22) "4572" 23) "role-reported" 24) "slave" 25) "role-reported-time" 26) "4930667" 27) "master-link-down-time" 28) "0" 29) "master-link-status" 30) "ok" 31) "master-host"

and many many more, currently it's failing to map those back to the objects using the code in ToObject.

        if (fieldValues.Length == 0)
            return default(T);
        var dict = new Dictionary<string, string>();
        for (int i = 0; i < fieldValues.Length; i += 2)
        {
            string key = fieldValues[i].ToString().Replace("-", String.Empty);
            if (key == "name")
                key = "Name";
            string value = fieldValues[i + 1].ToString();
            dict[key] = value;
        }
        return Serializer<T>.Deserialize(dict);

Above this there are a few lines that are commented out

        T obj = Activator.CreateInstance<T>();

        TypeConverter conv;
        PropertyInfo prop;
        string field, value;
        for (int i = 0; i < fieldValues.Length; i += 2)
        {
            field = fieldValues[i].ToString().Replace("-", String.Empty);
            value = fieldValues[i + 1].ToString();
            prop = typeof(T).GetProperty(field, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance);
            if (prop == null || !prop.CanWrite)
                continue;
            conv = TypeDescriptor.GetConverter(prop.PropertyType);
            if (!conv.CanConvertFrom(typeof(String)))
                continue;
            if (prop.PropertyType == typeof(Boolean) && value == "1")
                value = "true";
            prop.SetValue(obj, conv.ConvertFrom(value), null);
        }
        return obj;

All of which work a treat. I've swapped out the code again and it works again now. This probably isn't your fix, just bringing it to your attention.

— Reply to this email directly or view it on GitHubhttps://github.com/ctstone/csredis/issues/6 .