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

Need Code refactoring for better code reusability in StatusUpdaterBolt #1025

Closed
msghasan opened this issue Jan 4, 2023 · 11 comments
Closed
Labels

Comments

@msghasan
Copy link
Contributor

msghasan commented Jan 4, 2023

Issue 1028 solves the problem because the docId will be picked form the cache

@msghasan
Copy link
Contributor Author

msghasan commented Jan 4, 2023

I could not add the label as wish

@jnioche jnioche added the wish label Jan 4, 2023
@msghasan
Copy link
Contributor Author

msghasan commented Jan 5, 2023

@jnioche Can I provide the fix and raise a PR

@jnioche
Copy link
Contributor

jnioche commented Jan 5, 2023

Sure, this will help us understand what you want to achieve. As far as the doc ID is concerned, I'd rather we had the option to take it from the metadata, a bit like what is suggested in #671 for the IndexerBolt. This would give you the flexibility to generate the Id any way you like.

@msghasan
Copy link
Contributor Author

msghasan commented Jan 7, 2023

81b85a1

@msghasan
Copy link
Contributor Author

msghasan commented Jan 7, 2023

@jnioche I thought I would be able to create a new PR but my new commits went into and existing pr I have mapped the particular commit here.
Request you guys to verify my previous PR and also would be starting to work on Selenium Grid tomorrow. Please verify and approve the changes..
Regards

@jnioche
Copy link
Contributor

jnioche commented Jan 8, 2023

@msghasan it is not clear from the dangling commit you mentioned what you are trying to achieve and because it is not in a PR it makes it difficult to comment on specific changes.
In any case we will be addressing the doc id issue in #1028 so there is no point doing it in here.
I am not going to approve a commit which makes drastic changes to a key class without proper discussion on what it does etc...

@msghasan
Copy link
Contributor Author

msghasan commented Jan 8, 2023

@jnioche I only refactored the store method in parts so that anybody can reuse it as per their requirement when they extend the class StatusUpdaterBolt. I did not do any changes in the existing functionality.

@msghasan
Copy link
Contributor Author

msghasan commented Jan 8, 2023

@msghasan it is not clear from the dangling commit you mentioned what you are trying to achieve and because it is not in a PR it makes it difficult to comment on specific changes. In any case we will be addressing the doc id issue in #1028 so there is no point doing it in here. I am not going to approve a commit which makes drastic changes to a key class without proper discussion on what it does etc...

I wanted to create a PR for that but as an existing PR was opened by me for the archtype changes it got committed to that one because it was open. I have corrected the mistake and reverted the commit.. Will raise a new PR when the existing one gets approved..

@rzo1
Copy link
Contributor

rzo1 commented Jan 8, 2023

Doesn't #1028 solve it? If so, cool. If not, it might need more explanatio as it needs some more discussion / reasoning for changing core classes.

@msghasan
Copy link
Contributor Author

msghasan commented Jan 9, 2023

@jnioche @rzo1 Please ignore my previous message. I got confused by the issue no #1028. I will be deleting it and try to rephrase it again.
I can see you guys did not understood the problem correctly maybe I was not able to express correctly , so I will be re writing the description again for you guys to understand it. I will be deleting the previous comment I made.

@msghasan
Copy link
Contributor Author

msghasan commented Jan 9, 2023

Sorry for the comments lol I made a full of myself.. Understood where I was wrong.

@rzo1 rzo1 added the invalid label Jan 9, 2023
@rzo1 rzo1 closed this as completed Jan 9, 2023
@rzo1 rzo1 removed the wish label Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants