Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YCQL] Wrong port sent for NEW_NODE topology change events #2607

Closed
m-iancu opened this issue Oct 15, 2019 · 1 comment
Closed

[YCQL] Wrong port sent for NEW_NODE topology change events #2607

m-iancu opened this issue Oct 15, 2019 · 1 comment
Assignees

Comments

@m-iancu
Copy link
Contributor

m-iancu commented Oct 15, 2019

For each new node, we are actually sending the tserver hostport instead of the CQL one. Therefore, the host/IP is correct, but the port is wrong: typically 9100 (tserver default) instead of 9042 (CQL default).

Some drivers (e.g. the Java driver) were ignoring the port sent in the event anyway and
using the one set in config/constructor of the driver object -- so this was not practically an issue.
But others like Gocql seem to rely on it.

To replicate

1. Install gocql

go get github.com/yugabyte/gocql

2. Copy the code below into a file cql_example.go

package main;

import (
    "fmt"
    "log"
    "time"
    "strconv"

    "github.com/yugabyte/gocql"
)


func main() {

    // Connect to the cluster.
    cluster := gocql.NewCluster("127.0.0.1", "127.0.0.2", "127.0.0.3")

    // Use the same timeout as the Java driver.
    cluster.Timeout = 12 * time.Second

    // Create the session.
    session, _ := cluster.CreateSession()
    defer session.Close()

    // Set up the keyspace and table.
    if err := session.Query("CREATE KEYSPACE IF NOT EXISTS ybdemo").Exec(); err != nil {
        log.Fatal(err)
    }
    fmt.Println("Created keyspace ybdemo")


    if err := session.Query(`DROP TABLE IF EXISTS ybdemo.employee`).Exec(); err != nil {
        log.Fatal(err)
    }
    var createStmt = `CREATE TABLE ybdemo.employee (id int PRIMARY KEY,
                                                           name varchar,
                                                           age int,
                                                           language varchar)`;
    if err := session.Query(createStmt).Exec(); err != nil {
        log.Fatal(err)
    }
    fmt.Println("Created table ybdemo.employee")

    // Insert some rows into the table.
    for i := 0; i < 10000; i++ {
        var insertStmt string = "INSERT INTO ybdemo.employee(id, name, age, language)" +
            " VALUES (" + strconv.Itoa(i) + ", 'John', 35, 'Go')";
        if err := session.Query(insertStmt).Exec(); err != nil {
            log.Fatal(err)
        }
    }

}

3. Start a yb cluster

Use rf-3 and a low refresh interval to ensure we get topology change events often:

./bin/yb-ctl destroy && ./bin/yb-ctl create --rf 3 --tserver_flags="cql_nodelist_refresh_interval_secs=5" --master_flags="tserver_unresponsive_timeout_ms=5000"

4. Now run the app

go run -tags "gocql_debug" cql_example.go

This should output a lot of logging including lines like:

2019/10/14 17:29:27 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.2 port=9100]
2019/10/14 17:29:27 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.3 port=9100]
2019/10/14 17:29:27 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.1 port=9100]
2019/10/14 17:29:27 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.1 port=9100]
2019/10/14 17:29:27 gocql: handling frame: [topology_change change=MOVED_NODE host=127.0.0.1 port=9042]

Notice that above NEW_NODE events have the wrong 9100 port while the MOVED_NODE event has the correct 9042 port.

@m-iancu m-iancu self-assigned this Oct 15, 2019
m-iancu added a commit that referenced this issue Oct 15, 2019
Summary:
Previously, for each node node, we were sending the tserver port (typically `9100`).
Now, we correctly send the CQL port (typically `9042`) so that the drivers can handle
change correctly.

Note: Some drivers (e.g. the Java driver) were ignoring the port sent in the event and
using the one set in config/constructor of the driver object. But others like Gocql seem
to rely on it.

Test Plan:
Follow the instructions from #2607
and check that the ports in the output are not correct (`9042` instead of `9100`).

```
2019/10/14 17:55:50 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.1 port=9042]
2019/10/14 17:55:50 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.2 port=9042]
2019/10/14 17:55:50 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.1 port=9042]
2019/10/14 17:55:50 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.2 port=9042]
2019/10/14 17:55:50 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.3 port=9042]
2019/10/14 17:55:50 gocql: handling frame: [topology_change change=NEW_NODE host=127.0.0.3 port=9042]
2019/10/14 17:55:50 gocql: handling frame: [topology_change change=MOVED_NODE host=127.0.0.1 port=9042]
```

Reviewers: neil, sergei

Reviewed By: sergei

Subscribers: amitanand, kannan, yql

Differential Revision: https://phabricator.dev.yugabyte.com/D7397
@m-iancu
Copy link
Contributor Author

m-iancu commented Oct 15, 2019

Fixed by c5cffa2.

@m-iancu m-iancu closed this as completed Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant