Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

add back METRIC_BYTES_FROM_STMGRS #2035

Closed
wants to merge 11 commits into from
Closed

add back METRIC_BYTES_FROM_STMGRS #2035

wants to merge 11 commits into from

Conversation

huijunwu
Copy link
Member

@huijunwu huijunwu commented Jul 7, 2017

Related to #1908

Since the change from TupleStreamMessage to TupleStreamMessage2, the tuple structure information was lost, thus the following count cannot be fetched.

  • METRIC_BYTES_FROM_STMGRS
  • METRIC_BYTES_FROM_INSTANCES
  • METRIC_BYTES_TO_INSTANCES_LOST
  • METRIC_BYTES_TO_INSTANCES

Copy link
Contributor

@srkukarni srkukarni left a comment

Choose a reason for hiding this comment

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

Shouldn';t this be CachedByteSize?

@huijunw
Copy link
Contributor

huijunw commented Jul 11, 2017

I did not find the CachedByteSize() in protobuf.
There is SpaceUsed() which is noticeably slower than ByteSize()

@huijunw
Copy link
Contributor

huijunw commented Jul 14, 2017

thanks @objmagic

@objmagic
Copy link
Contributor

@huijunw we should use ByteSize instead of GetCachedSize. In fact, before 557c980, we use ByteSize only.

@huijunwu
Copy link
Member Author

huijunwu commented Aug 29, 2017

@objmagic see the previous comment, GetCachedSize was suggested @srkukarni

@objmagic
Copy link
Contributor

@srkukarni can you explain why we use GetCachedSize? I think its API description is not very clear. Will it return the correct current byte size of a message?

@srkukarni
Copy link
Contributor

GetCachedSize returns the previously calculated GetByteSize. Thus in order for GetCachedSize to be valid, some one should have called GetByteSize and the message should not have been altered since then.
I am seeing that no one is calling GetBytesSize on HeronTupleSet. This means that GetCachedSize is likely incorrect.
Here is a proposal

  1. In Stmgr-Server where stmgr first gets anything(either TupleStreamMessage from stmgr or HeronTupleSet from instance) we need to call GetByteSize().
  2. Rest other places GetCachedSize should return the right values.
    Can we see whats the perf impact of these changes?

@objmagic
Copy link
Contributor

objmagic commented Aug 29, 2017

@srkukarni makes sense. I was worried if we do not call ByteSize first we won't get correct value of GetCachedSize. Your proposal looks good. We need to carefully make sure ByteSize will be called at the very first. Then use GetCachedSize all the way until we release it.

For perf, we may need more time to fix the viz.

This proposal may help #2253 as well.

@huijunwu huijunwu closed this Aug 21, 2018
@huijunwu huijunwu deleted the METRIC_BYTES_FROM_STMGRS branch August 21, 2018 09:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants