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

SAI-4731 : supporting feature flag in solr #180

Open
wants to merge 7 commits into
base: fs/branch_9_3
Choose a base branch
from

Conversation

noblepaul
Copy link
Collaborator

@noblepaul noblepaul commented Feb 20, 2024

sample code

WatchedClusterProperties wcp =
        ch.getCoreContainer().getZkController().getWatchedClusterProperties();
    wcp.watchProperty(
        "prop1",
        (k, v) -> {
         //do something
        });
    wcp.watchProperty(
        "prop2",
        (k, v) -> {
          // do something
        });
        

@hiteshk25
Copy link
Collaborator

@noblepaul looks like host level support is not there yet?

@noblepaul
Copy link
Collaborator Author

@hiteshk25

per node properties ar implemented

(Map<String, Object>)
Utils.fromJSONString(
"{\n"
+ " \"watched-node-properties\" :{\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What all way we can define this?

  1. Can we differentiate between two components? (say, component-x, component-y)
  2. Can we define prop for component-x (node-level, otherwise-all, just-for-all) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not per component, it's just a property per node. May be we can prefix a component name in the property key

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need three cases here

  1. One can define properties at component level. While registering watch take component-name, if same exists then through the exception.
  2. Component will register default value, which we can update through ZK (or maybe later nice solr api)
    2.1. We can update component1-prop in ZK with node name, saying apply this value for that node. Then we should call listener for that component on that node only.

Side node, recently we introduced this behavior in prod, where we updated max-basic-queries limit on one solr-node. We watched that node for ~4/5 weeks and now we ready to update this limit on all the nodes. This solution require whole cluster restart, that's why we are looking for this feature in solr. Hope this will help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for example , if there is a property component-a.property-x that component may watch that property explicitly at load time. if no other component has registered a watch on that property, then no callbacks are given to other components. Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

f there is a property component-a.property-x that component may watch that property explicitly at load time.

As long as component is registered, it should get callback. do you think it should register property as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is no concept of a "component" is Solr. So, the system would not know whom to give a callback if it does not explicitly register for a callback

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's just name if we need to use feature flag. watch("test1", listener)

for (String k : newData.keySet()) {
if (knownData.containsKey(k)) continue;
isModified = true;
invokeListener(k, newData.get(k), propertiesListeners);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if prop is set for node1, then we need to invoke listener on that node only

Copy link
Collaborator Author

@noblepaul noblepaul Feb 28, 2024

Choose a reason for hiding this comment

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

yeah, that is what it does. You register the watcher from within a node and listeners are fired for the properties of that node only

Copy link
Collaborator

Choose a reason for hiding this comment

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

What will happens we update the property in ZK, will that cause listener-update-event on each node or all node?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean by each node and all node?

everyone who registered a listener should get a callback

Copy link
Collaborator

Choose a reason for hiding this comment

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

For example I want these property value for this node only

{
  component: "basiclimit", host:solr-1,property-x:value-y
} 

Then this should be invoked on host solr-1 only.

Copy link
Collaborator Author

@noblepaul noblepaul Mar 1, 2024

Choose a reason for hiding this comment

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

when the property is defined for solr-1 , you get a calback on solr-1 only.
It would look something like this. Inside a node, you can only watch properties of that node

{
  "watched-node-properties" :{
    "solr-1" : {
        "basiclimit.property-x":"value-y"
    }
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, let's give demo on this in Monday's sync. thanks!

…to both global and node property. if the same property is present in global and node values, global value is ignored
@hiteshk25
Copy link
Collaborator

@noblepaul let's plan for updated demo on Monday's sync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants