Skip to content

SAMZA-2301: Add non-null checks in JobModel read control-flow in standalone.#1139

Closed
shanthoosh wants to merge 1 commit intoapache:masterfrom
shanthoosh:master
Closed

SAMZA-2301: Add non-null checks in JobModel read control-flow in standalone.#1139
shanthoosh wants to merge 1 commit intoapache:masterfrom
shanthoosh:master

Conversation

@shanthoosh
Copy link
Copy Markdown
Contributor

No description provided.

@shanthoosh shanthoosh requested a review from rmatharu-zz August 15, 2019 01:59
for (TaskModel taskModel : containerModel.getTasks().values()) {
taskToProcessorId.put(taskModel.getTaskName(), containerModel.getId());
for (SystemStreamPartition partition : taskModel.getSystemStreamPartitions()) {
taskToSSPs.computeIfAbsent(taskModel.getTaskName(), k -> new ArrayList<>());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can replace with
taskToSSPs.putIfAbsent(taskModel.getTaskName(), new ArrayList<>()); ?

Copy link
Copy Markdown
Contributor

@rmatharu-zz rmatharu-zz left a comment

Choose a reason for hiding this comment

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

one minor comment, feel free to fix and commit

LOG.info("pid=" + processorId + ": new JobModel is available. Version =" + jobModelVersion + "; JobModel = " + newJobModel);

if (!newJobModel.getContainers().containsKey(processorId)) {
if (newJobModel != null && !newJobModel.getContainers().containsKey(processorId)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Until this change, the else code path corresponds to the case where the processor is ensured to be part of the new job model generated. However, with this change, we blur the distinction between processor being part of the job model vs job model unavailable.
I suppose the by product of that is you can potentially have a stream processor running that is not part of the quorum or doesn't have any work to do.
If that is intended, please add a comment and link the existing ticket.

@prateekm
Copy link
Copy Markdown
Contributor

prateekm commented Oct 8, 2019

@shanthoosh Is this ready to be merged?

Replace ConfigBasedUDFResolver with the UDF resolver based on the reflections.
@shanthoosh
Copy link
Copy Markdown
Contributor Author

shanthoosh commented Oct 15, 2019

Yes, @prateekm . This branch had a bunch of conflicts since it was open for a long period of time. Tried resolving them, but it kind of pulled in unrelated changes. Will open a new request in the future with the merge-conflicts resolved.

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.

4 participants