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

Copy operatorSummaries before merging stats with running drivers #773

Merged

Conversation

2 participants
@sopel39
Copy link
Member

commented May 14, 2019

PipelineContext contains list of running Driver instances.
All fields in PipelineContext are thread safe. However
getPipelineStats and driverFinished methods modify/read
those fields multiple times, therefore they are racy.
This causes PipelineStats to be consistent only when
there are no running drivers.
When driver is finished its OperatorStats stats are
accumulated in PipelineContext map. Because of that
in previous code version getPipelineStats could double
account for DriverStats (once from accumulated fields
and once from a copy of list of running drivers).

In this PR code is changed so that PipelineContext
can only underaccount for finished driver stats.
This is more natural and less confusing as cumulative OperatorStats
of a pipeline do not exceed accumulated PipelineStats themselves.

Copy operatorSummaries before merging stats with running drivers
PipelineContext contains list of running Driver instances.
All fields in PipelineContext are thread safe. However
getPipelineStats and driverFinished methods modify/read
those fields multiple times, therefore they are racy.
This causes PipelineStats to be consistent only when
there are no running drivers.
When driver is finished its OperatorStats stats are
accumulated in PipelineContext map. Because of that
in previous code version getPipelineStats could double
account for DriverStats (once from accumulated fields
and once from a copy of list of running drivers).

In this PR code is changed so that PipelineContext
can only underaccount for finished driver stats.
This is more natural and less confusing as cumulative OperatorStats
of a pipeline do not exceed accumulated PipelineStats themselves.

@sopel39 sopel39 requested a review from dain May 14, 2019

@cla-bot cla-bot bot added the cla-signed label May 14, 2019

@sopel39

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Previously we could have seen stats like:

OperatorStats from PipelineStats
              "operatorSummaries" : [ {
                "stageId" : 2,
                "pipelineId" : 0,
                "operatorId" : 0,
                "planNodeId" : "3",
                "operatorType" : "ScanFilterAndProjectOperator",
                "totalDrivers" : 186,
                "addInputCalls" : 200570,
                "addInputWall" : "3.34m",
                "addInputCpu" : "0.00ns",
                "physicalInputDataSize" : "2.16GB",
                "physicalInputPositions" : 198493799,
                "internalNetworkInputDataSize" : "0B",

PipelineStats
            "fullyBlocked" : false,
            "blockedReasons" : [ ],
            "physicalInputDataSize" : "1.58GB",
            "physicalInputPositions" : 145379736,
            "internalNetworkInputDataSize" : "0B",
            "internalNetworkInputPositions" : 0,

that is really confusing.

@dain

dain approved these changes May 14, 2019

@sopel39 sopel39 merged commit fce37d8 into prestosql:master May 14, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details

@sopel39 sopel39 deleted the starburstdata:ks/fix_pipeline_stats_accounting branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.