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

[Refactoring] Abstract AsyncShardFetch cache to allow restructuring for batch mode #12440

Closed
amkhar opened this issue Feb 23, 2024 · 6 comments
Closed
Assignees
Labels
bug Something isn't working Cluster Manager

Comments

@amkhar
Copy link
Contributor

amkhar commented Feb 23, 2024

Describe the bug

Overall project issue : #8098

AsyncShardFetch class cache is like private final Map<String, NodeEntry<T>> cache = new HashMap<>();
This is just a map which will always store the full response (T) returned from the data nodes when we want to get the metadata of shards after node-left/join. The responses from new transport return the data for all the shards in a map like Map<ShardId, ShardResponse>
So when we fill the cache it'll store the data like

Map<String, Map<ShardId, ShardResponse>>

This map stores the data for every nodeId, so ShardId will get repeated for every node. What we should do is not store the shardId in repeated manner but use some array to store just the data and keep ShardId mapping separately.

Current Map private final Map<String, NodeEntry<T>> cache can not support this use case.

Related issue for adding a new caching strategy for a batch of shards #12248

Related component

Cluster Manager

To Reproduce

N/A

Expected behavior

Current cache can not support other strategies. So abstracting out this implementation in a separate class will help other caching strategies to be implemented easily.
To have that restructuring in place, I'm suggesting to have three simple methods like

initData - Initialize the cache entry
getData - get the data from the cache
putData - store the data in the cache

Each caching strategy should implement these methods and handle how things should be done. And driver class should just care about node level response and executing the end to end flow (common functionalities to be put in base class to avoid duplication).

This way, new caching strategy for batch of shards would be able to put the response data in relevant format like in an array and get it accordingly. Existing implementation can be same.

Additional Details

N/A

@peternied
Copy link
Member

[Triage - attendees 1 2 3 4 5]
@amkhar Thanks for creating this issue; however, it isn't being accepted due to the issue not having enough context on its own please recreate with more details. Please feel free to open a new issue after addressing the reason.

@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Cluster Manager Project Board Feb 28, 2024
@amkhar
Copy link
Contributor Author

amkhar commented Feb 28, 2024

Sure @peternied , I'll add more details in this issue itself (if it's okay to re-open with more details ?).

Just curious if you went through #12248 to understand how we want to implement the cache for new transport actions being written for a batch of shards ?

For that implementation to be clean, first we need to modify the current cache (map) into a class so existing implementation and new implementation can be put in separate child classes.

@peternied
Copy link
Member

Just curious if you went through...

This issue was reviewed by the triage team during the core triage meeting, we read the issue as it is. While additional context / links are useful the general guidance is the problem and expected end state are clearly described in the issue.

if it's okay to re-open with more details

If you have the ability to do so, please feel free to reopen.

@amkhar
Copy link
Contributor Author

amkhar commented Feb 28, 2024

Thanks for confirmation.
Added more details :)

@amkhar amkhar reopened this Feb 28, 2024
@github-project-automation github-project-automation bot moved this from ✅ Done to 🏗 In progress in Cluster Manager Project Board Feb 28, 2024
@peternied
Copy link
Member

[Triage - attendees 1 2 3 4 5]
@amkhar Thanks for the additional context - this issue looks good.

@amkhar
Copy link
Contributor Author

amkhar commented Mar 14, 2024

Completed by #12441

@amkhar amkhar closed this as completed Mar 14, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Cluster Manager Project Board Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Cluster Manager
Projects
Status: ✅ Done
Development

No branches or pull requests

2 participants