[GitHub] [nifi] nabmoh123 commented on pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors

2021-04-07 Thread GitBox


nabmoh123 commented on pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#issuecomment-814967533


   > I still think that a custom validate method makes sense. Especially to 
make the processor invalid if brokers, host, and port are all set. Because in 
that case it could confuse the users wrt to which properties are actually used. 
I acknowledge this is explained in the properties description but this would 
make things more "obvious".
   > 
   > We do that for some processors where it's possible to configure the same 
"thing" via multiple properties after we added some new properties over time 
and we kept the old ones to not break backward compatibility.
   > 
   > That's my only remaining comment, latest changes look good to me.
   
   I could implement this, but it would require me to take off the default 
values for Host Name and Port. Otherwise, whenever Brokers is set, Host Name 
will take on its default value and cause a warning to appear. I'm not sure 
which is the lesser of two evils here, removing old functionality or not adding 
in clear and concise validation to ensure users understand which property is 
used.


-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org




[GitHub] [nifi] nabmoh123 commented on pull request #4939: NIFI-8341 Support Multi Hosts in AMQP Processors

2021-04-06 Thread GitBox


nabmoh123 commented on pull request #4939:
URL: https://github.com/apache/nifi/pull/4939#issuecomment-814001461


   > Thanks for this improvement. Could you add a customValidate method to 
check the configuration of the processor and make it invalid if both brokers 
and host properties are set simultaneously, or if none of the properties are 
set? You may find similar pieces of code in other processors.
   > 
   > Right now, one could leave all properties empty, the processor would be 
valid and throw NPE exceptions since the `connection` object would not be set.
   
   The Host and Port have default values set. So NiFi reenters the default 
values in if you leave the properties empty. Is it still worth adding in the 
customValidate for this?


-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org