[Fix-16382]Fix the bug of duplicate error reporting of procedure parameters#16420
[Fix-16382]Fix the bug of duplicate error reporting of procedure parameters#16420wangxj3 wants to merge 0 commit intoapache:devfrom
Conversation
wangxj3
requested review from
Gallardot,
SbloodyS,
caishunfeng and
zhuangchong
as code owners
...src/main/test/org/apache/dolphinscheduler/plugin/task/procedure/ProcedureParametersTest.java
Outdated
Show resolved
Hide resolved
...src/main/test/org/apache/dolphinscheduler/plugin/task/procedure/ProcedureParametersTest.java
Outdated
Show resolved
Hide resolved
|
| if (!varPool.isEmpty()) { | ||
| for (Property info : varPool) { | ||
| if (pop.equals(info.getProp())) { | ||
| varPool.remove(info); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
You should fix this in AbstractParameters since this is used by more than just one task type.
There was a problem hiding this comment.
other task type use the mothod of AbstractParameters.dealOutParam .only procedure task handle output parameters in this way.
There was a problem hiding this comment.
What I mean is remove duplicated value of varPool in AbstractParameters.
There was a problem hiding this comment.
What I mean is remove duplicated value of
varPoolinAbstractParameters.
this is the method merge the out params in AbstractParameters, the inputparams of this method is varpool and outProperty,This is sequential. It will be inserted into the map according to the order of varpool and then outProperty. The map will filter out duplicate values, and because outProperty is behind, it will overwrite the duplicate values of varpool.Do you mean this？
There was a problem hiding this comment.
So the mergeVarPool of AbstractParameters does not need to be modified
There was a problem hiding this comment.
After I take a deep look at it, I think we should make the following changes.
- Deduplication should be performed at the place where varpool was initialized.
- Add duplicate check at the front-end.
- Add duplicate check at the backend-end api.
There was a problem hiding this comment.
I think parameter updates should be a normal business scenario
2 participants
Purpose of the pull request
fix #16382
Brief change log
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
Pull Request Notice
Pull Request Notice
If your pull request contain incompatible change, you should also add it to
docs/docs/en/guide/upgrede/incompatible.md