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

Dispatch App : Fix setting of dispatch plug from command line #5434

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

ivanimanishi
Copy link
Member

The same dispatcher can be added to the dispatchers list multiple times, once from the dispatcher parameter, and again from the alternateDispatchers parameter.

That in turn resulted in the wrong dispatcher object being the one updated by the settings from the command line.

The fix here is to remove duplicate objects from the list, so there is only one dispatcher for a given name.

In our specific scenario, we were making the following sort of command line call:

gaffer dispatch -gui -tasks GafferJabuka.Nodes.Executable.InputProfileDispatcher -show InputProfileDispatcher -dispatcher Farm -settings -FarmDispatcher.farm.taskType '"InputProfileDispatcher"'

And the -FarmDispatcher.farm.taskType plug wasn't being set.

I considered a way not to add the same dispatcher twice, but couldn't find a nice way of doing that.

So instead I just remove the duplicates before processing the list further.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@ivanimanishi ivanimanishi force-pushed the dispatchAppFix branch 2 times, most recently from 18a3db5 to 0164f41 Compare August 23, 2023 21:40
@johnhaddon
Copy link
Member

Thanks Ivan! I'm quitr happy to merge this as-is, but see what you think to the following :

I considered a way not to add the same dispatcher twice, but couldn't find a nice way of doing that.

I always considered Dispatcher.createMatching() to be a weird API addition. It's a trivial combination of registeredDispatchers() and StringAlgo::matchMultiple(), and its limited potential use cases don't justify cluttering the API with it. I think this has been borne out by the fact that we still only have one exact call to it after all these years, the one that is in the way here in the dispatch app.

So could we deprecate that, and do something like this instead?

if args["gui"].value and len( args["alternateDispatchers"] ) : 
    for name in Dispatcher.registeredDispatchers() :
         if name != dispatcherType and StringAlgo.matchMultiple( name, args["alternateDispatchers".value ) :
            dispatchers.append( Dispatcher.create( name )

It's slightly less code overall, lets us tidy up the API, and doesn't have the inelegant property of making things just to throw them away.

The same dispatcher can be added to the `dispatchers` list multiple
times, once from the `dispatcher` parameter, and again from the
`alternateDispatchers` parameter.

That in turn resulted in the wrong dispatcher object being the one
updated by the settings from the command line.

The fix here is to prevent the duplicate addition of the same
dispatcher.
@ivanimanishi
Copy link
Member Author

@johnhaddon , I've updated the PR following your suggestion.

Note that I haven't deprecated Dispatcher.createMatching(). I'll leave that to you.

@johnhaddon johnhaddon merged commit 8555f97 into GafferHQ:1.2_maintenance Aug 29, 2023
4 checks passed
johnhaddon added a commit to johnhaddon/gaffer that referenced this pull request Aug 29, 2023
This was a premature addition to the API - to our knowledge it only ever had one use case, and this eventually turned out to be poorly served by it anyway - see GafferHQ#5434. It's better to conform to the more minimal registry API used by other classes, and then let people compose their own logic on top.
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.

None yet

2 participants