FAQ
I submitted 3 CLs back to back this morning and the third ended up in the
datastore with a valid name but a 0 time and num. I think that made adg's
watcher.go exit. I tried running a new watcher.go and it exited too:
"dashboard: in datastore with zero Num".

The two CLs that followed those 3 also got 0 time and num. I deleted all
three of those datastore entries and then build.golang.org was showing a
page containing only "datastore: no such entity".

Then I started watcher.go again. This time it did not exit and uploaded the
five missing CLs. That seems to have brought build.golang.org back to life.

We really need to figure out where the Num=0 entries are coming from. They
had only name, num, and time set; nothing else. The fact that the two
subsequent entries also got Num=0 suggest that once the watcher had gone
away, something was creating those entries.

I think the perf dashboard is the problem. In particular, when the normal
build dashboard is stopped the perf dashboard still seems to race ahead and
tell us about build breakages. That suggests it is not getting told about
CLs from the standard dashboard dispatcher. If that's the case, then it may
try to record a perf result for a commit before the standard dispatcher has
recorded that the commit even exists.

Looking at the app code seems to confirm this theory:

tx := func(c appengine.Context) error {
// check Package exists
if _, err := GetPackage(c, res.PackagePath); err != nil {
return fmt.Errorf("GetPackage: %v", err)
}
// put Result
if _, err := datastore.Put(c, res.Key(c), res); err != nil {
return fmt.Errorf("putting Result: %v", err)
}
// add Result to Commit
com := &Commit{PackagePath: res.PackagePath, Hash: res.Hash}
if err := com.AddResult(c, res); err != nil {
return fmt.Errorf("AddResult: %v", err)
}
// Send build failure notifications, if necessary.
// Note this must run after the call AddResult, which
// populates the Commit's ResultData field.
return notifyOnFailure(c, com, res.Builder)
}

There is no check that the Commit actually exists. Doing this AddResult
here will create the Commit if it does not exist already, and the Commit
will end up with a zeroed Num, Time, and everything else.

Please fix this.

Thanks.
Russ

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Search Discussions

  • Dmitry Vyukov at Oct 7, 2014 at 5:09 pm

    On Tue, Oct 7, 2014 at 8:50 PM, Russ Cox wrote:
    I submitted 3 CLs back to back this morning and the third ended up in the
    datastore with a valid name but a 0 time and num. I think that made adg's
    watcher.go exit. I tried running a new watcher.go and it exited too:
    "dashboard: in datastore with zero Num".

    The two CLs that followed those 3 also got 0 time and num. I deleted all
    three of those datastore entries and then build.golang.org was showing a
    page containing only "datastore: no such entity".

    Then I started watcher.go again. This time it did not exit and uploaded the
    five missing CLs. That seems to have brought build.golang.org back to life.

    We really need to figure out where the Num=0 entries are coming from. They
    had only name, num, and time set; nothing else. The fact that the two
    subsequent entries also got Num=0 suggest that once the watcher had gone
    away, something was creating those entries.

    I think the perf dashboard is the problem. In particular, when the normal
    build dashboard is stopped the perf dashboard still seems to race ahead and
    tell us about build breakages. That suggests it is not getting told about
    CLs from the standard dashboard dispatcher. If that's the case, then it may
    try to record a perf result for a commit before the standard dispatcher has
    recorded that the commit even exists.
    How is this possible? AddCommitToPerfTodo is called in addCommit
    transaction that creates the commit with proper Num.
    Looking at the app code seems to confirm this theory:

    tx := func(c appengine.Context) error {
    // check Package exists
    if _, err := GetPackage(c, res.PackagePath); err != nil {
    return fmt.Errorf("GetPackage: %v", err)
    }
    // put Result
    if _, err := datastore.Put(c, res.Key(c), res); err != nil {
    return fmt.Errorf("putting Result: %v", err)
    }
    // add Result to Commit
    com := &Commit{PackagePath: res.PackagePath, Hash: res.Hash}
    if err := com.AddResult(c, res); err != nil {
    return fmt.Errorf("AddResult: %v", err)
    }
    // Send build failure notifications, if necessary.
    // Note this must run after the call AddResult, which
    // populates the Commit's ResultData field.
    return notifyOnFailure(c, com, res.Builder)
    }
    This is from resultHandler. resultHandler is called by normal
    builders, not perf builders.


    There is no check that the Commit actually exists. Doing this AddResult here
    will create the Commit if it does not exist already, and the Commit will end
    up with a zeroed Num, Time, and everything else.

    Please fix this.

    Thanks.
    Russ
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Russ Cox at Oct 7, 2014 at 5:59 pm
    Okay, that code is correct then, because it only runs when a builder has
    been told about the commit by the dashboard.

    Is it true that the perf builders go off and build a commit without being
    told about the commit by the dashboard? If so, that seems like it must be
    the race causing the problem.

    The perf result recording does the same thing as the other recording, but
    because there is no guarantee the commit exists, it might create a shell
    commit accidentally:

    // addPerfResult creates PerfResult and updates Commit, PerfTodo,
    // PerfMetricRun and PerfConfig. Must be executed within a transaction.
    func addPerfResult(c appengine.Context, r *http.Request, req *PerfRequest)
    error {
    // check Package exists
    p, err := GetPackage(c, "")
    if err != nil {
    return fmt.Errorf("GetPackage: %v", err)
    }
    // add result to Commit
    com := &Commit{Hash: req.Hash}
    if err := com.AddPerfResult(c, req.Builder, req.Benchmark); err != nil {
    return fmt.Errorf("AddPerfResult: %v", err)
    }

    // add the result to PerfResult
    res := &PerfResult{CommitHash: req.Hash}
    if err := datastore.Get(c, res.Key(c), res); err != nil {
    return fmt.Errorf("getting PerfResult: %v", err)
    }
    present := res.AddResult(req)
    if _, err := datastore.Put(c, res.Key(c), res); err != nil {
    return fmt.Errorf("putting PerfResult: %v", err)
    }

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Russ Cox at Oct 7, 2014 at 6:03 pm
    CL 154080043 might fix this.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 7, 2014 at 6:08 pm

    On Tue, Oct 7, 2014 at 9:59 PM, Russ Cox wrote:
    Okay, that code is correct then, because it only runs when a builder has
    been told about the commit by the dashboard.

    Is it true that the perf builders go off and build a commit without being
    told about the commit by the dashboard?
    It's not true AFAICT. A commit is added to perf-todo when it's created in DB.

    If so, that seems like it must be
    the race causing the problem.

    The perf result recording does the same thing as the other recording, but
    because there is no guarantee the commit exists, it might create a shell
    commit accidentally:

    // addPerfResult creates PerfResult and updates Commit, PerfTodo,
    // PerfMetricRun and PerfConfig. Must be executed within a transaction.
    func addPerfResult(c appengine.Context, r *http.Request, req *PerfRequest)
    error {
    // check Package exists
    p, err := GetPackage(c, "")
    if err != nil {
    return fmt.Errorf("GetPackage: %v", err)
    }
    // add result to Commit
    com := &Commit{Hash: req.Hash}
    if err := com.AddPerfResult(c, req.Builder, req.Benchmark); err != nil {
    return fmt.Errorf("AddPerfResult: %v", err)
    }

    // add the result to PerfResult
    res := &PerfResult{CommitHash: req.Hash}
    if err := datastore.Get(c, res.Key(c), res); err != nil {
    return fmt.Errorf("getting PerfResult: %v", err)
    }
    present := res.AddResult(req)
    if _, err := datastore.Put(c, res.Key(c), res); err != nil {
    return fmt.Errorf("putting PerfResult: %v", err)
    }
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 7, 2014 at 6:49 pm

    On Tue, Oct 7, 2014 at 10:07 PM, Dmitry Vyukov wrote:
    On Tue, Oct 7, 2014 at 9:59 PM, Russ Cox wrote:
    Okay, that code is correct then, because it only runs when a builder has
    been told about the commit by the dashboard.

    Is it true that the perf builders go off and build a commit without being
    told about the commit by the dashboard?
    It's not true AFAICT. A commit is added to perf-todo when it's created in DB.

    However. Perf-todo records commit nums (not commit hashes). And as far
    as I understand commit nums were manually shuffled some time ago. It
    can be possible that perf-todo now contains "future" commit nums. I
    don't know how exactly database was updated.
    One consequence of the update is that old commits are now not
    benchmarked. Another is that perf graphs contain huge "holes". I am
    curious whether perf data is now associated with wrong commits... as
    that association is also commit.num-based... that would be very bad.



    If so, that seems like it must be
    the race causing the problem.

    The perf result recording does the same thing as the other recording, but
    because there is no guarantee the commit exists, it might create a shell
    commit accidentally:

    // addPerfResult creates PerfResult and updates Commit, PerfTodo,
    // PerfMetricRun and PerfConfig. Must be executed within a transaction.
    func addPerfResult(c appengine.Context, r *http.Request, req *PerfRequest)
    error {
    // check Package exists
    p, err := GetPackage(c, "")
    if err != nil {
    return fmt.Errorf("GetPackage: %v", err)
    }
    // add result to Commit
    com := &Commit{Hash: req.Hash}
    if err := com.AddPerfResult(c, req.Builder, req.Benchmark); err != nil {
    return fmt.Errorf("AddPerfResult: %v", err)
    }

    // add the result to PerfResult
    res := &PerfResult{CommitHash: req.Hash}
    if err := datastore.Get(c, res.Key(c), res); err != nil {
    return fmt.Errorf("getting PerfResult: %v", err)
    }
    present := res.AddResult(req)
    if _, err := datastore.Put(c, res.Key(c), res); err != nil {
    return fmt.Errorf("putting PerfResult: %v", err)
    }
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Russ Cox at Oct 7, 2014 at 7:28 pm

    On Tue, Oct 7, 2014 at 2:49 PM, Dmitry Vyukov wrote:
    On Tue, Oct 7, 2014 at 10:07 PM, Dmitry Vyukov wrote:
    On Tue, Oct 7, 2014 at 9:59 PM, Russ Cox wrote:
    Okay, that code is correct then, because it only runs when a builder has
    been told about the commit by the dashboard.

    Is it true that the perf builders go off and build a commit without
    being
    told about the commit by the dashboard?
    It's not true AFAICT. A commit is added to perf-todo when it's created
    in DB.

    However. Perf-todo records commit nums (not commit hashes). And as far
    as I understand commit nums were manually shuffled some time ago. It
    can be possible that perf-todo now contains "future" commit nums. I
    don't know how exactly database was updated.
    One consequence of the update is that old commits are now not
    benchmarked. Another is that perf graphs contain huge "holes". I am
    curious whether perf data is now associated with wrong commits... as
    that association is also commit.num-based... that would be very bad.
    There are some big holes now. When I was manually fixing the datastore I
    set the next number to 100000 to make sure there weren't any stale numbers
    floating around. The perf drawing code will have to figure out how to deal
    with that. I am sorry. Perhaps it can fetch all the Num from the recent
    Commits and thereby build a mapping from the sparse Num ordering to a dense
    graph ordering.

    I think I did find the source of the Num=0 Commits though. It looks like
    one of the two code paths that sends build breakage mails was doing a
    datastore.Put with a Commit that only had the Hash and
    FailNotificationSent. I didn't try to fix the code path but I made the
    datastore.Put not execute if the rest of the information is missing. This
    will result in duplicate mails about build breakages but it should stop the
    corruption. I'm going to leave it to someone else more familiar with the
    code to make the correct fix. CL 154080043 has this and is running on
    build.golang.org right now.

    Russ

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Russ Cox at Oct 7, 2014 at 7:30 pm

    On Tue, Oct 7, 2014 at 3:28 PM, Russ Cox wrote:

    However. Perf-todo records commit nums (not commit hashes). And as far
    as I understand commit nums were manually shuffled some time ago. It
    can be possible that perf-todo now contains "future" commit nums. I
    don't know how exactly database was updated.
    One consequence of the update is that old commits are now not
    benchmarked. Another is that perf graphs contain huge "holes". I am
    curious whether perf data is now associated with wrong commits... as
    that association is also commit.num-based... that would be very bad.
    This is a serious mistake. The association should be hash based. The nums
    are not reliable.

    Russ

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Russ Cox at Oct 7, 2014 at 8:24 pm
    To get back to the original build failure, my CL has confirmed the theory
    about commit mails:

    2014-10-07 12:08:00.165 refusing to notify with com={PackagePath:
    Hash:c7857de341434044e406f03ca4c29dbcb66e55df ParentHash: Num:0 User: Desc:
    Time:0001-01-01 00:00:00 +0000 UTC NeedsBenchmarking:false TryPatch:false
    ResultData:[] PerfResults:[] FailNotificationSent:false}
    goroutine 357 [running]:
    build.commonNotify(0x7f39ecfb42f8, 0xc0109afd20, 0xc010a15c60,
    0xc010ae4be0, 0x12, ...)
    build/notify.go:222 +0x3ed
    build.sendPerfFailMail(0x7f39ecfb42f8, 0xc0109afd20, 0xc010ae4be0, 0x12,
    0xc010866dc0, ...)
    build/notify.go:216 +0x334
    build.checkPerfChanges(0x7f39ecfb42f8, 0xc0109afd20, 0xc010be9000,
    0xc01093fe70, 0xc010ae4be0, ...)
    build/handler.go:706 +0x5e2
    build.addPerfResult(0x7f39ecfb42f8, 0xc0109afd20, 0xc010be9000,
    0xc011563620, 0x177ef30, ...)
    build/handler.go:640 +0x753
    build.func·005(0x7f39ecfb42f8, 0xc0109afd20, 0xc, 0x177ef30)
    build/handler.go:606 +0x5e
    appengine/datastore.runOnce(0x7f39ecfb3668, 0xc010c26f00, 0x7f39ece23590,
    0x0, 0x0, ...)
    go/src/pkg/appengine/datastore/transaction.go:74 +0x2dc
    appengine/datastore.RunInTransaction(0x7f39ecfb3668, 0xc010c26f00,
    0x7f39ece23590, 0x0, 0xc010bc1480, ...)
    go/src/pkg/appengine/datastore/transaction.go:119 +0x14e
    build.perfResultHandler(0xc010be9000, 0x0, 0x0, 0x0, 0x0)
    build/handler.go:608 +0x5a5
    build.func·006(0x7f39ecfb3630, 0xc010c26f00, 0xc010be9000)
    build/handler.go:815 +0x322
    net/http.HandlerFunc.ServeHTTP(0xc010968710, 0x7f39ecfb3630, 0xc010c26f00,
    0xc010be9000)
    go/src/pkg/net/http/server.go:1220 +0x55
    net/http.(*ServeMux).ServeHTTP(0xc0108554e0, 0x7f39ecfb3630, 0xc010c26f00,
    0xc010be9000)
    go/src/pkg/net/http/server.go:1496 +0x19a
    appengine_internal.executeRequestSafely(0xc010c26f00, 0xc010be9000)
    go/src/pkg/appengine_internal/api_prod.go:280 +0x9f
    appengine_internal.(*server).HandleRequest(0x1a57e40, 0xc0109c06c0,
    0xc011561900, 0xc011561a80, 0x0, ...)
    go/src/pkg/appengine_internal/api_prod.go:214 +0xc35
    reflect.Value.call(0x169f3c0, 0x1a57e40, 0x138, 0x172ae10, 0x4, ...)
    go/src/pkg/reflect/value.go:474 +0xe23
    reflect.Value.Call(0x169f3c0, 0x1a57e40, 0x138, 0x7f39ece23f38, 0x3, ...)
    go/src/pkg/reflect/value.go:345 +0xb5

    Andrew, I have a watcher running in a loop in a screen session, because I
    believe yours died due to the Num=0 that had been created.

    Russ

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Andrew Gerrand at Oct 8, 2014 at 2:14 am

    On 8 October 2014 07:24, Russ Cox wrote:

    Andrew, I have a watcher running in a loop in a screen session, because I
    believe yours died due to the Num=0 that had been created.

    That's unnecessary. The commit watcher running on compute engine is also
    running in a loop in a screen session (Brad and I will dockerize it this
    week), and is still running. You can shut yours down now. Thanks.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 8, 2014 at 8:36 am
    Please drop all PerfTodo entities from the datastore. They will be
    recreated with correct commit nums on next todo request.
    However, well... I am not sure how corrupted is the database now...
    that worked for me on an integral database... CommitRuns are probably
    all corrupted as well... We now need to drop all perf data from the
    datastore and then run the update script again to recreate correct
    CommitRuns.


    On Wed, Oct 8, 2014 at 6:13 AM, Andrew Gerrand wrote:
    On 8 October 2014 07:24, Russ Cox wrote:

    Andrew, I have a watcher running in a loop in a screen session, because I
    believe yours died due to the Num=0 that had been created.

    That's unnecessary. The commit watcher running on compute engine is also
    running in a loop in a screen session (Brad and I will dockerize it this
    week), and is still running. You can shut yours down now. Thanks.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Andrew Gerrand at Oct 8, 2014 at 2:36 pm

    On 8 October 2014 19:36, Dmitry Vyukov wrote:

    Please drop all PerfTodo entities from the datastore.

    We'll need to push a version of the app with some code that does this.
    There's no way to bulk delete from the admin console.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 8, 2014 at 3:05 pm
    Datastore Admin tab, check PerfTodo, click Delete Entities.

    On Wed, Oct 8, 2014 at 6:35 PM, Andrew Gerrand wrote:
    On 8 October 2014 19:36, Dmitry Vyukov wrote:

    Please drop all PerfTodo entities from the datastore.

    We'll need to push a version of the app with some code that does this.
    There's no way to bulk delete from the admin console.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Andrew Gerrand at Oct 8, 2014 at 3:11 pm
    Well, how fancy! That must be newish.
    On 9 October 2014 02:05, Dmitry Vyukov wrote:

    Datastore Admin tab, check PerfTodo, click Delete Entities.

    On Wed, Oct 8, 2014 at 6:35 PM, Andrew Gerrand wrote:
    On 8 October 2014 19:36, Dmitry Vyukov wrote:

    Please drop all PerfTodo entities from the datastore.

    We'll need to push a version of the app with some code that does this.
    There's no way to bulk delete from the admin console.
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 8, 2014 at 8:32 am

    On Tue, Oct 7, 2014 at 11:30 PM, Russ Cox wrote:
    On Tue, Oct 7, 2014 at 3:28 PM, Russ Cox wrote:

    However. Perf-todo records commit nums (not commit hashes). And as far
    as I understand commit nums were manually shuffled some time ago. It
    can be possible that perf-todo now contains "future" commit nums. I
    don't know how exactly database was updated.
    One consequence of the update is that old commits are now not
    benchmarked. Another is that perf graphs contain huge "holes". I am
    curious whether perf data is now associated with wrong commits... as
    that association is also commit.num-based... that would be very bad.

    This is a serious mistake. The association should be hash based. The nums
    are not reliable.

    Nums are more reliable than hashes. They are generated in a
    transaction by incrementing a counter. If that is not reliable, then
    there is nothing reliable in such database.
    What is a serious mistake is manual updates to database that break
    data integrity.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Russ Cox at Oct 8, 2014 at 9:11 pm

    On Wed, Oct 8, 2014 at 4:32 AM, Dmitry Vyukov wrote:

    Nums are more reliable than hashes. They are generated in a
    transaction by incrementing a counter. If that is not reliable, then
    there is nothing reliable in such database.
    The change graph is a dag, not a line. Relying on a sequence number of the
    nodes in the dag as being meaningful other than as one of many possible
    orderings of the nodes is fundamentally broken. For example, the
    [dev.power64] and [dev.garbage] and [release-branch.go1.3] entries are all
    interleaved with the default branch in this sequence numbering. You have to
    do some kind of processing to filter them out and recompact the sequence
    once it contains just the CLs you care about. Given that, it shouldn't
    matter that there are now also gaps. Think about the missing numbers as
    entries on another branch with a bunch of commits you don't care about.

    What is a serious mistake is manual updates to database that break
    data integrity.
    I apologize for fixing the database and making the dashboard work again.
    Just trying to get my work done.

    Russ

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 9, 2014 at 10:05 am

    On Thu, Oct 9, 2014 at 1:11 AM, Russ Cox wrote:
    On Wed, Oct 8, 2014 at 4:32 AM, Dmitry Vyukov wrote:

    Nums are more reliable than hashes. They are generated in a
    transaction by incrementing a counter. If that is not reliable, then
    there is nothing reliable in such database.

    The change graph is a dag, not a line. Relying on a sequence number of the
    nodes in the dag as being meaningful other than as one of many possible
    orderings of the nodes is fundamentally broken.
    I am not doing this. I am not ordering whole dag, I am ordering only
    one path. Looks fine to me.


    For example, the
    [dev.power64] and [dev.garbage] and [release-branch.go1.3] entries are all
    interleaved with the default branch in this sequence numbering. You have to
    do some kind of processing to filter them out and recompact the sequence
    once it contains just the CLs you care about. Given that, it shouldn't
    matter that there are now also gaps. Think about the missing numbers as
    entries on another branch with a bunch of commits you don't care about.
    How would hashes make anything simpler or more correct or reliable?

    What is a serious mistake is manual updates to database that break
    data integrity.

    I apologize for fixing the database and making the dashboard work again.
    Just trying to get my work done.

    I don't knot what is the state of the database, but if Commit.Num's
    were manually updated then I suspect it is still corrupted.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Gustavo Niemeyer at Oct 9, 2014 at 10:32 am
    Hey Dmitry,

    On Thu, Oct 9, 2014 at 12:05 PM, 'Dmitry Vyukov' via golang-dev
    wrote:
    I am not doing this. I am not ordering whole dag, I am ordering only
    one path. Looks fine to me.
    Is this useful:

    "Revision numbers referring to changesets are very likely to be
    different in another copy of a repository. Do not use them to talk
    about changesets with other people. Use the changeset ID instead."

    http://mercurial.selenic.com/wiki/RevisionNumber
    How would hashes make anything simpler or more correct or reliable?
    The hash uniquely describes the content itself, which is generally the
    intended semantics in those cases.


    gustavo @ http://niemeyer.net

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Andrew Gerrand at Oct 9, 2014 at 10:52 am
    Gustavo,

    The numbers ("Num") we're talking about here are numbers internal to the
    dashboard.

    We know the Mercurial revision numbers are useless.

    Andrew
    On 9 October 2014 12:31, Gustavo Niemeyer wrote:

    Hey Dmitry,

    On Thu, Oct 9, 2014 at 12:05 PM, 'Dmitry Vyukov' via golang-dev
    wrote:
    I am not doing this. I am not ordering whole dag, I am ordering only
    one path. Looks fine to me.
    Is this useful:

    "Revision numbers referring to changesets are very likely to be
    different in another copy of a repository. Do not use them to talk
    about changesets with other people. Use the changeset ID instead."

    http://mercurial.selenic.com/wiki/RevisionNumber
    How would hashes make anything simpler or more correct or reliable?
    The hash uniquely describes the content itself, which is generally the
    intended semantics in those cases.


    gustavo @ http://niemeyer.net
    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 9, 2014 at 10:55 am

    On Thu, Oct 9, 2014 at 2:31 PM, Gustavo Niemeyer wrote:
    Hey Dmitry,

    On Thu, Oct 9, 2014 at 12:05 PM, 'Dmitry Vyukov' via golang-dev
    wrote:
    I am not doing this. I am not ordering whole dag, I am ordering only
    one path. Looks fine to me.
    Is this useful:

    "Revision numbers referring to changesets are very likely to be
    different in another copy of a repository. Do not use them to talk
    about changesets with other people. Use the changeset ID instead."

    http://mercurial.selenic.com/wiki/RevisionNumber
    The commit nums that we are talking about are generated by the database.

    How would hashes make anything simpler or more correct or reliable?
    The hash uniquely describes the content itself, which is generally the
    intended semantics in those cases.

    The app does not calculate/verify hashes, it merely uses them as
    unique identifier (just as commit nums). Moreover, perf dashboard
    really needs ordering. Drawing graphs in an abstract hash space makes
    little sense.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Russ Cox at Oct 14, 2014 at 5:23 pm

    On Thu, Oct 9, 2014 at 6:05 AM, Dmitry Vyukov wrote:
    On Thu, Oct 9, 2014 at 1:11 AM, Russ Cox wrote:
    On Wed, Oct 8, 2014 at 4:32 AM, Dmitry Vyukov wrote:

    Nums are more reliable than hashes. They are generated in a
    transaction by incrementing a counter. If that is not reliable, then
    there is nothing reliable in such database.

    The change graph is a dag, not a line. Relying on a sequence number of the
    nodes in the dag as being meaningful other than as one of many possible
    orderings of the nodes is fundamentally broken.
    I am not doing this. I am not ordering whole dag, I am ordering only
    one path. Looks fine to me.
    That's fine, then. The numbers are good for ordering individual branches.

    For example, the
    [dev.power64] and [dev.garbage] and [release-branch.go1.3] entries are all
    interleaved with the default branch in this sequence numbering. You have to
    do some kind of processing to filter them out and recompact the sequence
    once it contains just the CLs you care about. Given that, it shouldn't
    matter that there are now also gaps. Think about the missing numbers as
    entries on another branch with a bunch of commits you don't care about.
    How would hashes make anything simpler or more correct or reliable?
    The hashes are guaranteed not to change, even if we find a bug and need to
    recompute the server-local numbering. PerfTodo's should be using hashes for
    exactly that reason.

    Russ

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Dmitry Vyukov at Oct 15, 2014 at 8:15 am

    On Tue, Oct 14, 2014 at 9:23 PM, Russ Cox wrote:
    On Thu, Oct 9, 2014 at 6:05 AM, Dmitry Vyukov wrote:
    On Thu, Oct 9, 2014 at 1:11 AM, Russ Cox wrote:
    On Wed, Oct 8, 2014 at 4:32 AM, Dmitry Vyukov <dvyukov@google.com>
    wrote:
    Nums are more reliable than hashes. They are generated in a
    transaction by incrementing a counter. If that is not reliable, then
    there is nothing reliable in such database.

    The change graph is a dag, not a line. Relying on a sequence number of
    the
    nodes in the dag as being meaningful other than as one of many possible
    orderings of the nodes is fundamentally broken.
    I am not doing this. I am not ordering whole dag, I am ordering only
    one path. Looks fine to me.

    That's fine, then. The numbers are good for ordering individual branches.
    For example, the
    [dev.power64] and [dev.garbage] and [release-branch.go1.3] entries are
    all
    interleaved with the default branch in this sequence numbering. You have
    to
    do some kind of processing to filter them out and recompact the sequence
    once it contains just the CLs you care about. Given that, it shouldn't
    matter that there are now also gaps. Think about the missing numbers as
    entries on another branch with a bunch of commits you don't care about.
    How would hashes make anything simpler or more correct or reliable?

    The hashes are guaranteed not to change, even if we find a bug and need to
    recompute the server-local numbering. PerfTodo's should be using hashes for
    exactly that reason.

    Either don't do this, or do this correctly. There _is_ a way to alter
    commit nums w/o breaking integrity. There is nothing magical is the
    way database uses commit nums.

    If there is some bug and we need to do some update, you don't know
    what database structure will tolerate an arbitrary future update
    better. What is a builder sends in wrong commit hashes?

    And I want to note that usage of commit nums is not just my caprice.
    The design discussions were public on this list. My initial design
    used commit hashes. The problem that Andrew raised is that selecting
    hundreds and thousands of results to draw several metrics over several
    Go releases will kill the appengine database to death. You can see
    this in action with the 100'000 commit num gap. After that we've
    changed the design so that drawing of N lines requires fetching of
    only N+1 entities.
    So what you are proposing is just not going to work.

    --
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 7, '14 at 4:50p
activeOct 15, '14 at 8:15a
posts22
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase