You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2024/09/17 19:46:18 UTC
[PR] Improve/standardize rate limiting logic in Monitor [accumulo]
DomGarguilo opened a new pull request, #4894:
URL: https://github.com/apache/accumulo/pull/4894
This PR uses Guavas `Suppliers.memoizeWithExpiration()` for caching, replacing the manual caching logic. This allows us to:
* Remove `synchronized` from the get methods since `Suppliers.memoizeWithExpiration()` is thread safe
* Remove the class-level maps that were previously being used for caching. Now, the `fetch()` methods that update the data, directly return the data instead of using these maps. These `fetch()` methods are called directly from the suppliers now.
* Remove the age-off logic that was being done in the `fetch()` methods. It should be noted that the functionality has slightly changed. Since we just re-create a new map whenever the data is refreshed in the `fetch()` methods, there are no old values that need to be aged 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2358914756
> > Remove the age-off logic that was being done in the fetch() methods. It should be noted that the functionality has slightly changed. Since we just re-create a new map whenever the data is refreshed in the fetch() methods, there are no old values that need to be aged off.
>
> That is a nice simplification.
Yea definitely nice to be able to remove some code. Wasn't sure if this would be an issue though since before, the maps would contain values that would only be aged off after they were 15 minutes old. And now, since the data is recreated once every minute, all of the returned data will only ever be that old. Not too sure what the implications of this are or what the old values were being used for.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2359189212
> Wasn't sure if this would be an issue though since before, the maps would contain values that would only be aged off after they were 15 minutes old.
I misread that code when I was reviewing. I thought they were aging off after 15 milliseconds, so did not think it was an issue. But its actually 15 minutes. It would be good to understand that change in behavior a bit better. So it seems like some things used to hang around for 15 minutes in the monitor display even after they were gone?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2369048752
Regarding my previous comment:
I looked into the other spots where data was being pulled from these maps in the `Monitor` code (tserverScans, sserverScans, and allCompactions). Unless I am missing something, the only one that seems to use this stale data before it is aged off is the scan servers scenario outlined above. This leads me to believe that we might not need to keep this age off functionality unless it is deemed necessary for the scan server scans info.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2364662287
@DomGarguilo for these these dead servers that hang around in the monitor for 15mins, do you know if their last contact time continues to increase in monitor display?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2597165843
Oops, yep I will make sure to do that first thing tomorrow. I didn’t merge
right away since there was another merge in progress and then forgot to
follow up.
On Thu, Jan 16, 2025 at 7:12 PM Christopher Tubbs ***@***.***>
wrote:
> @DomGarguilo <https://github.com/DomGarguilo> It looks like this wasn't
> merged fully into the main branch yesterday, and there are some outstanding
> conflicts with the Monitor. @keith-turner
> <https://github.com/keith-turner> merged it up to 3.1 today, resolving
> the conflicts in 3.1. If you are able, it would be great if you could merge
> 3.1 into main and resolve the conflicts from this PR on the Monitor (and
> also double check the merge from 2.1 into 3.1 to make sure that that merge
> went okay).
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/4894#issuecomment-2597163236>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ALMD2IM4NT5DZ5VTOH24NLT2LBDGHAVCNFSM6AAAAABOMFWZICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJXGE3DGMRTGY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2597163236
@DomGarguilo It looks like this wasn't merged fully into the main branch yesterday, and there are some outstanding conflicts with the Monitor. @keith-turner merged it up to 3.1 today, resolving the conflicts in 3.1. If you are able, it would be great if you could merge 3.1 into main and resolve the conflicts from this PR on the Monitor (and also double check the merge from 2.1 into 3.1 to make sure that that merge went okay).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2364050238
> So it seems like some things used to hang around for 15 minutes in the monitor display even after they were gone?
Yea, so after looking at the code a bit more I think that the purpose of the age-off logic was to remove entries whose server was no longer present. The code that collects this data iterates through each server returned from `context.instanceOperations().getTabletServers()` and uses the HostAndPort of that server as the key in the map. So if there is new data for that server, the old data will just be replaced, but in the case where there is some data for a server and then that server dies, then it will not be replaced by anything. So after 15 mins, if the server did not come back up, those entries will be aged off. It seems like we want to keep this functionality so I'm going to work on adding this age-off functionality back.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #4894:
URL: https://github.com/apache/accumulo/pull/4894
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve/standardize rate limiting logic in Monitor [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4894:
URL: https://github.com/apache/accumulo/pull/4894#issuecomment-2369012988
> @DomGarguilo for these these dead servers that hang around in the monitor for 15mins, do you know if their last contact time continues to increase in monitor display?
Okay so i was a bit mistaken. The info for servers that have dies did stick around in the map and then it depends on what the calling code does with it. For example, `ScansResource` sets up the rest endpoint for scans and it filters the data to only return scans from tservers that are alive, where as for scan server scans, it will return all of the data in the map. This means that if a scan server dies, as long as the 15 min age-off window has not passed, scans that were running on that scan server will still be displayed. This is just one example of how this potentially stale data is used in the monitor. I am taking a look at the other usages of this data that is kept around. I am not sure why we want to keep this data around on the monitor for scan servers but not for tservers.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org