You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "neilramaswamy (via GitHub)" <gi...@apache.org> on 2024/09/10 22:32:56 UTC
[PR] [SPARK-49592] Modify OperatorStateMetadataV2Reader to return the right metadata for the provided batchId [spark]
neilramaswamy opened a new pull request, #48066:
URL: https://github.com/apache/spark/pull/48066
### What changes were proposed in this pull request?
Fixes a bug in the `OperatorStateMetadataV2Reader` where the `read` method, if called in isolation, could return the wrong metadata.
### Why are the changes needed?
The `OperatorStateMetadataV2Reader` has _end-behavior_ that is desirable, but it only "accidentally" works. Currently, if you have the offsets `0 1 2` and ask for `batchId = 3`, the `read` method could return you the operator state metadata for `2` instead of throwing an error. In practice, this doesn't happen, since we have a check in the constructor that the `batchId` provided must be less than or equal to the last offset.
Really, what we want is that if `read` ever gets called and the asked-for `batchId` doesn't exist, an error should be thrown there instead of silently returning wrong metadata.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
All existing UTs should pass.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49592][SS] Modify OperatorStateMetadataV2Reader to return the right metadata for the provided batchId [spark]
Posted by "neilramaswamy (via GitHub)" <gi...@apache.org>.
neilramaswamy commented on PR #48066:
URL: https://github.com/apache/spark/pull/48066#issuecomment-2345425741
Yeah, sorry, I discussed with anishshri-db (not tagging) yesterday about this and while he initially agreed (just like your original comment, haha), he then later had the same realization. I think the confusion is that the reader for a given `batchId` doesn't actually return the metadata for `batchId`—it returns the metadata _before_ `batchId`. And the reader has the additional constraint that the offset for `batchId` must be present.
I think, if anything, a comment should be added to this class. But otherwise, this is a non-issue, so I'm going to close it.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49592] Modify OperatorStateMetadataV2Reader to return the right metadata for the provided batchId [spark]
Posted by "ericm-db (via GitHub)" <gi...@apache.org>.
ericm-db commented on PR #48066:
URL: https://github.com/apache/spark/pull/48066#issuecomment-2342538881
Sorry, thought about this a little more. I don't think this is actually fixing anything, in that we don't write metadata for every single batch, only every run.
Say we have run a query for 5 batches, and then stopped it, so we wrote the metadata file for batch 0.
If we want to get the metadata that is valid for batch 3 (metadata doesn't change mid-run), we would want to read the metadata file that was written for batch 0. I think your change actually breaks this behavior...
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49592][SS] Modify OperatorStateMetadataV2Reader to return the right metadata for the provided batchId [spark]
Posted by "neilramaswamy (via GitHub)" <gi...@apache.org>.
neilramaswamy closed pull request #48066: [SPARK-49592][SS] Modify OperatorStateMetadataV2Reader to return the right metadata for the provided batchId
URL: https://github.com/apache/spark/pull/48066
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49592] Modify OperatorStateMetadataV2Reader to return the right metadata for the provided batchId [spark]
Posted by "ericm-db (via GitHub)" <gi...@apache.org>.
ericm-db commented on PR #48066:
URL: https://github.com/apache/spark/pull/48066#issuecomment-2342532300
@neilramaswamy Can you please add a test for this, looks good to me otherwise
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org