You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/11/02 15:28:58 UTC
[GitHub] [qpid-dispatch] ganeshmurthy opened a new pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
ganeshmurthy opened a new pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416
… (-DQD_MEMORY_STATS=OFF)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-957878563
hmmm... I wonder if it would be simpler to make QD_MEMORY_STATS non-configurable? IOW, get rid of the flag and just always provide memory statistics. It's pretty obvious we haven't tested QD_MEMORY_STATS=OFF as part of our CI.
Just an opinion, thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-957887593
> hmmm... I wonder if it would be simpler to make QD_MEMORY_STATS non-configurable? IOW, get rid of the flag and just always provide memory statistics. It's pretty obvious we haven't tested QD_MEMORY_STATS=OFF as part of our CI.
>
> Just an opinion, thoughts?
I am all for getting rid of the flag once and for all and always provide memory statistics but first I want to understand why this flag was introduced in the first place ?
If we decide to keep this flag, we need to augment CI (preferably GHA) to test with QD_MEMORY_STATS=OFF. That will add *yet another* build type to the already long list of tests we run, so I am inclined to say let's just get rid of this flag.
@ted-ross what do you think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ted-ross commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
ted-ross commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-958124314
> > hmmm... I wonder if it would be simpler to make QD_MEMORY_STATS non-configurable? IOW, get rid of the flag and just always provide memory statistics. It's pretty obvious we haven't tested QD_MEMORY_STATS=OFF as part of our CI.
> > Just an opinion, thoughts?
>
> I am all for getting rid of the flag once and for all and always provide memory statistics but first I want to understand why this flag was introduced in the first place ? If we decide to keep this flag, we need to augment CI (preferably GHA) to test with QD_MEMORY_STATS=OFF. That will add _yet another_ build type to the already long list of tests we run, so I am inclined to say let's just get rid of this flag.
>
> @ted-ross what do you think ?
+1 on removing the flag. This was first introduced due to concern that the stats collection might be costly. I don't believe that it has been identified as a performance/memory bottleneck in any of the investigations we've done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit merged pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#discussion_r741984270
##########
File path: tools/qdstat
##########
@@ -743,10 +743,7 @@ class BusManager(object):
row.append(PlainNum(t.batchesRebalancedToGlobal))
rows.append(row)
pooled_total += (t.typeSize * t.totalAllocFromHeap)
- if not rows:
Review comment:
You might want to leave this check since qdstat may be used against older routers that were built without enabling memory stats.
Highly unlikely I would imagine, but who knows?
##########
File path: tools/qdstat
##########
@@ -743,10 +743,7 @@ class BusManager(object):
row.append(PlainNum(t.batchesRebalancedToGlobal))
rows.append(row)
pooled_total += (t.typeSize * t.totalAllocFromHeap)
- if not rows:
Review comment:
You might want to leave this check since qdstat may be used against older routers that were built without enabling memory stats.
Highly unlikely I would imagine, but who knows?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-957878563
hmmm... I wonder if it would be simpler to make QD_MEMORY_STATS non-configurable? IOW, get rid of the flag and just always provide memory statistics. It's pretty obvious we haven't tested QD_MEMORY_STATS=OFF as part of our CI.
Just an opinion, thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#discussion_r741984270
##########
File path: tools/qdstat
##########
@@ -743,10 +743,7 @@ class BusManager(object):
row.append(PlainNum(t.batchesRebalancedToGlobal))
rows.append(row)
pooled_total += (t.typeSize * t.totalAllocFromHeap)
- if not rows:
Review comment:
You might want to leave this check since qdstat may be used against older routers that were built without enabling memory stats.
Highly unlikely I would imagine, but who knows?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit merged pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-957887593
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-958128206
> Let's do this the simple way and remove the flag.
Sounds good. I will add a commit to this PR removing the flag. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-957878563
hmmm... I wonder if it would be simpler to make QD_MEMORY_STATS non-configurable? IOW, get rid of the flag and just always provide memory statistics. It's pretty obvious we haven't tested QD_MEMORY_STATS=OFF as part of our CI.
Just an opinion, thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ted-ross commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
ted-ross commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-958124314
> > hmmm... I wonder if it would be simpler to make QD_MEMORY_STATS non-configurable? IOW, get rid of the flag and just always provide memory statistics. It's pretty obvious we haven't tested QD_MEMORY_STATS=OFF as part of our CI.
> > Just an opinion, thoughts?
>
> I am all for getting rid of the flag once and for all and always provide memory statistics but first I want to understand why this flag was introduced in the first place ? If we decide to keep this flag, we need to augment CI (preferably GHA) to test with QD_MEMORY_STATS=OFF. That will add _yet another_ build type to the already long list of tests we run, so I am inclined to say let's just get rid of this flag.
>
> @ted-ross what do you think ?
+1 on removing the flag. This was first introduced due to concern that the stats collection might be costly. I don't believe that it has been identified as a performance/memory bottleneck in any of the investigations we've done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#issuecomment-957887593
> hmmm... I wonder if it would be simpler to make QD_MEMORY_STATS non-configurable? IOW, get rid of the flag and just always provide memory statistics. It's pretty obvious we haven't tested QD_MEMORY_STATS=OFF as part of our CI.
>
> Just an opinion, thoughts?
I am all for getting rid of the flag once and for all and always provide memory statistics but first I want to understand why this flag was introduced in the first place ?
If we decide to keep this flag, we need to augment CI (preferably GHA) to test with QD_MEMORY_STATS=OFF. That will add *yet another* build type to the already long list of tests we run, so I am inclined to say let's just get rid of this flag.
@ted-ross what do you think ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit merged pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1416: DISPATCH-1958: Fixed error in qdstat when memory stats are turned off…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1416:
URL: https://github.com/apache/qpid-dispatch/pull/1416#discussion_r741984270
##########
File path: tools/qdstat
##########
@@ -743,10 +743,7 @@ class BusManager(object):
row.append(PlainNum(t.batchesRebalancedToGlobal))
rows.append(row)
pooled_total += (t.typeSize * t.totalAllocFromHeap)
- if not rows:
Review comment:
You might want to leave this check since qdstat may be used against older routers that were built without enabling memory stats.
Highly unlikely I would imagine, but who knows?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org