New issue
Advanced search Search tips

Issue 907292 link

Starred by 2 users
Status: Assigned
Owner:
akes...@chromium.org
Cc:
akes...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

qscheduler: decouple proto representation from internal representation

Project Member Reported by akes...@chromium.org, Nov 21 
The current implementation of qslib/scheduler and qslib/reconciler uses a proto-defined struct to represent the state.

This is suboptiomal for a few reasons:
 - in exposes internal aspects of state that don't need to be exported symbols
 - it adds cruft to the namespace of those packages
 - it makes it hard to change the storage/persistence layer for these APIs, becasue clients are aware of the internal representation


This should be cleaned up by putting protos into their own packages.

Comment 1 by ayatane@chromium.org, Nov 27

Status: Assigned (was: Untriaged)

Comment 2 by akes...@chromium.org, Dec 18

Cc: akes...@chromium.org
 Issue 914945  has been merged into this issue.
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 18 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/77914a4019e48582a27eb7d4190c2c449688aa04

commit 77914a4019e48582a27eb7d4190c2c449688aa04
Author: Aviv Keshet <akeshet@chromium.org>
Date: Tue Dec 18 01:20:52 2018

qscheduler: separate Scheduler struct from its proto representation

No behavioral change.

This CL is part of a series of refactor CLs that will separate Scheduler
and Reconciler from their proto representation, and move the protos to
their own package.

BUG=chromium:907292
TEST=existing tests pass

Change-Id: Id3410c266a3851db73447d995972542a1ce5b468
Reviewed-on: https://chromium-review.googlesource.com/c/1378787
Commit-Queue: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Allen Li <ayatane@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19620}
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/appengine/qscheduler-swarming/app/config/config.pb.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/qscheduler/qslib/scheduler/scheduler.proto
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/appengine/qscheduler-swarming/app/entities/entities.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/appengine/qscheduler-swarming/app/frontend/qscheduler_admin.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/qscheduler/qslib/scheduler/prioritize.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/qscheduler/qslib/scheduler/scheduler_test.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/qscheduler/qslib/scheduler/prioritize_test.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/appengine/qscheduler-swarming/app/frontend/qscheduler.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/qscheduler/qslib/scheduler/scheduler.pb.go
[modify] https://crrev.com/77914a4019e48582a27eb7d4190c2c449688aa04/go/src/infra/qscheduler/qslib/scheduler/scheduler.go
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 18 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/0b7895ff4daf583fa08582530ee88198c6397410

commit 0b7895ff4daf583fa08582530ee88198c6397410
Author: Aviv Keshet <akeshet@chromium.org>
Date: Tue Dec 18 01:24:22 2018

qscheduler: rename State to StateProto

No behavioral change.

This is part of a multi-CL refactor to detach proto representation from
go API.

BUG=chromium:907292
TEST=existing tests pass

Change-Id: I8d1eedd0831a5a2d7e0b335aea831080936d7b21
Reviewed-on: https://chromium-review.googlesource.com/c/1378797
Commit-Queue: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Allen Li <ayatane@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19621}
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/state.proto
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/types.go
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/scheduler.proto
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/state.pb.go
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/state_test.go
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/prioritize.go
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/state.go
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/scheduler_test.go
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/scheduler.pb.go
[modify] https://crrev.com/0b7895ff4daf583fa08582530ee88198c6397410/go/src/infra/qscheduler/qslib/scheduler/scheduler.go
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 18 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/71f41e7f69261844dfa69c330a69db584fb0d2a0

commit 71f41e7f69261844dfa69c330a69db584fb0d2a0
Author: Aviv Keshet <akeshet@chromium.org>
Date: Tue Dec 18 21:37:53 2018

qscheduler: decouple qslib API from State proto representation

No behavioral change, though some of the tests had to be modified
because they relied on proto representation.

The API still depends on Config proto representation, though the
dependence is not as intertwined. This will be fixed in a future CL.

This (in the end, much bigger than I wanted) CL extracts a lot of the
proto depenence out of the qslib API, deleting several modules and
moving a bunch of code around in the process.

BUG=chromium:907292
TEST=tests pass

Change-Id: I8fe607ce9b43ad690c296e91c8078bf4617aa6bd
Reviewed-on: https://chromium-review.googlesource.com/c/1378796
Commit-Queue: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Allen Li <ayatane@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19643}
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/account/account_test.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/state.proto
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/vector/vector.pb.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/state_test.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/state.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/appengine/qscheduler-swarming/api/qscheduler/v1/admin.pb.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/scheduler_test.go
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/vector/gen.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/worker.proto
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/account/gen.go
[add] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/vector.go
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/vector/vector_test.go
[add] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/types_test.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/types.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/task.pb.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/appengine/qscheduler-swarming/api/qscheduler/v1/admin.proto
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/vector/vector.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/helpers.go
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/account/account.pb.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/scheduler.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/state.pb.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/task.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/worker.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/reconciler/reconciler_test.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/doc_test.go
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/account/account.proto
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/appengine/qscheduler-swarming/api/qscheduler/v1/pb.discovery.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/prioritize.go
[delete] https://crrev.com/c1f706a24bef14b3f318af0897e1af59cdbff618/go/src/infra/qscheduler/qslib/types/vector/vector.proto
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/config.pb.go
[add] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/account_test.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/config.proto
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/worker.pb.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/prioritize_test.go
[modify] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/task.proto
[rename] https://crrev.com/71f41e7f69261844dfa69c330a69db584fb0d2a0/go/src/infra/qscheduler/qslib/scheduler/account.go
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 19 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/a15a00031ee27415e0fa43f5d56e9010aae38945

commit a15a00031ee27415e0fa43f5d56e9010aae38945
Author: Aviv Keshet <akeshet@chromium.org>
Date: Wed Dec 19 19:24:46 2018

qscheduler: turn Assignment from proto to regular go struct

Assignments are never serlialized to proto, and having this type as a
proto is inconvenient.

BUG=chromium:907292
TEST=None

Change-Id: Iaba016d0526da55e1b53e6052cec28174273ff35
Reviewed-on: https://chromium-review.googlesource.com/c/1383375
Commit-Queue: Aviv Keshet <akeshet@chromium.org>
Auto-Submit: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19674}
[delete] https://crrev.com/fe836559b510b72b487dfcd5eb06beafe97b1dc9/go/src/infra/qscheduler/qslib/scheduler/assignment.pb.go
[modify] https://crrev.com/a15a00031ee27415e0fa43f5d56e9010aae38945/go/src/infra/qscheduler/qslib/scheduler/scheduler.go
[modify] https://crrev.com/a15a00031ee27415e0fa43f5d56e9010aae38945/go/src/infra/qscheduler/qslib/scheduler/state_test.go
[modify] https://crrev.com/a15a00031ee27415e0fa43f5d56e9010aae38945/go/src/infra/qscheduler/qslib/scheduler/state.go
[modify] https://crrev.com/a15a00031ee27415e0fa43f5d56e9010aae38945/go/src/infra/qscheduler/qslib/scheduler/scheduler_test.go
[modify] https://crrev.com/a15a00031ee27415e0fa43f5d56e9010aae38945/go/src/infra/qscheduler/qslib/scheduler/prioritize_test.go
[modify] https://crrev.com/a15a00031ee27415e0fa43f5d56e9010aae38945/go/src/infra/qscheduler/qslib/reconciler/reconciler.go
[delete] https://crrev.com/fe836559b510b72b487dfcd5eb06beafe97b1dc9/go/src/infra/qscheduler/qslib/scheduler/assignment.proto
►

Sign in to add a comment