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

Ensure logTailer returns records in time order #6612

Merged
merged 6 commits into from Nov 25, 2016

Conversation

babbageclunk
Copy link
Contributor

Having the separate sort by id worked by coincidence when records were
inserted in time order, but now that old records can be added by
migration the ids don't match up with times so records come back out of order.

Add id to the index instead and use one sort.

QA:
Deployed ubuntu to a new model
Noted the earliest log message for the model
Bootstrapped another controller
Migrated the model to the new controller
Checked that the earliest log message was the same

@babbageclunk
Copy link
Contributor Author

Hmm, will this behave alright when there's an upgrade? I guess the first open post-upgrade will create the new index. Does there need to be something more explicit than this?

Having the separate sort by id worked by coincidence when records were
inserted in time order, but now that old records can be added by
migration the ids don't match up with times. Add id to the index
instead and use one sort.
state.InitDbLogs will create a new one on e,t,_id, but the old one needs
to be cleaned up at upgrade time.
@babbageclunk
Copy link
Contributor Author

!!build!!

@babbageclunk
Copy link
Contributor Author

Doing a manual test of the upgrade now.

@babbageclunk
Copy link
Contributor Author

Upgrade worked fine.
Before:

juju:PRIMARY> db.logs.getIndexes()
[
        {
                "v" : 1,
                "key" : {
                        "_id" : 1
                },
                "name" : "_id_",
                "ns" : "logs.logs"
        },
        {
                "v" : 1,
                "key" : {
                        "e" : 1,
                        "t" : 1
                },
                "name" : "e_1_t_1",
                "ns" : "logs.logs"
        },
        {
                "v" : 1,
                "key" : {
                        "e" : 1,
                        "n" : 1
                },
                "name" : "e_1_n_1",
                "ns" : "logs.logs"
        }
]

After:

juju:PRIMARY> db.logs.getIndexes()
[
        {
                "v" : 1,
                "key" : {
                        "_id" : 1
                },
                "name" : "_id_",
                "ns" : "logs.logs"
        },
        {
                "v" : 1,
                "key" : {
                        "e" : 1,
                        "n" : 1
                },
                "name" : "e_1_n_1",
                "ns" : "logs.logs"
        },
        {
                "v" : 1,
                "key" : {
                        "e" : 1,
                        "t" : 1,
                        "_id" : 1
                },
                "name" : "e_1_t_1__id_1",
                "ns" : "logs.logs"
        }
]

@babbageclunk
Copy link
Contributor Author

Gah. Not sure about these tests checking the values of constants.

@babbageclunk
Copy link
Contributor Author

!!build!!

if err == nil {
return nil
}
if queryErr, ok := err.(*mgo.QueryError); ok {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking you could check the output of Indexes() and then decide whether to drop, but this is fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that at first, but this seemed less racy. Although it does depend on looking at the error message.

Manual testing showed that the upgrade step failed when the index wasn't
present, until the code check was removed.
@babbageclunk
Copy link
Contributor Author

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Nov 25, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 25d8b1e into juju:develop Nov 25, 2016
@babbageclunk babbageclunk deleted the debug-log-order branch November 25, 2016 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants