You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "ddebowczyk92 (via GitHub)" <gi...@apache.org> on 2023/09/21 15:23:46 UTC
[GitHub] [beam] ddebowczyk92 opened a new pull request, #28590: Make SerializableConfiguration cacheable
ddebowczyk92 opened a new pull request, #28590:
URL: https://github.com/apache/beam/pull/28590
This PR addresses #28589
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ddebowczyk92 commented on pull request #28590: Make SerializableConfiguration cacheable
Posted by "ddebowczyk92 (via GitHub)" <gi...@apache.org>.
ddebowczyk92 commented on PR #28590:
URL: https://github.com/apache/beam/pull/28590#issuecomment-1731113436
`assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ddebowczyk92 commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "ddebowczyk92 (via GitHub)" <gi...@apache.org>.
ddebowczyk92 commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337361747
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
I think it's hard to identify all the places where `Configuration` could be modified after the serialization cache is initialized. We can assume that anytime `Configuration` is accessed by calling `SerializableConfiguration#get()` it might get modified therefore making `serializableCache` store expired/invalid data. To address this concern, I added cache invalidation at the beginning of the `SerializableConfiguration#get()` method in a PR update.
I tested this change on the same job I described in https://github.com/apache/beam/issues/28589 and it still works
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #28590: Make SerializableConfiguration cacheable
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28590:
URL: https://github.com/apache/beam/pull/28590#issuecomment-1731115879
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @Abacn for label java.
R: @Abacn for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn merged pull request #28590: Make SerializableConfiguration cacheable
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #28590:
URL: https://github.com/apache/beam/pull/28590
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make SerializableConfiguration cacheable [beam]
Posted by "kellen (via GitHub)" <gi...@apache.org>.
kellen commented on PR #28590:
URL: https://github.com/apache/beam/pull/28590#issuecomment-1828503916
Ah apologies, I did not read all the comments here.
In scio we have a bunch of tests running in parallel which previously revealed some unsafe (shared) usages of other structures inside beam, so I presume this is the same kind of thing.
It is not clear to me where this shared usage is taking place. I suspect that there are multiple `get` calls somewhere, potentially in parallel, that are invalidating the cache in the middle of the `writeExternal` call. IMO synchronizing on serializedCache would be a sufficient fix, since that would avoid errors during write, even if the class overall is not considered thread-safe.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337416209
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
thanks. shall we invalidate the cache in readExternal also - where conf is re-constructed
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1336118021
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
is there any chance that conf gets changed (e.g. readExternal call) so we should invalidate (set null) the cache?
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
is there any chance that conf gets changed (e.g. readExternal call) so we should invalidate (set null) the cache at corresponding place?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ddebowczyk92 commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "ddebowczyk92 (via GitHub)" <gi...@apache.org>.
ddebowczyk92 commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337361747
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
I think it's hard to identify all the places where `Configuration` could be modified after the serialization cache is initialized. We can assume that anytime `Configuration` is accessed by calling `SerializableConfiguration#get()` it might get modified therefore making `serializableCache` store expired/invalid data. To address this concern, I added cache invalidation at the beginning of the SerializableConfiguration#get() method in a PR update.
I tested this change on the same job I described in https://github.com/apache/beam/issues/28589 and it still works
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337654125
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
> because the implementation of SerializableConfiguration isn't thread-safe.
Thanks for explanation, then current implementation looks sufficient
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make SerializableConfiguration cacheable [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #28590:
URL: https://github.com/apache/beam/pull/28590#issuecomment-1828482369
> This PR has introduced a race between the setting of `serializationCache` and the write. We are seeing flaky tests in scio (tracking issue [spotify/scio#5099](https://github.com/spotify/scio/issues/5099))
>
> I am not sure why the cache should be cleared on `get` or `readExternal`, but if that is necessary `serializationCache` accesses should be synchronized
It sounds the concern in https://github.com/apache/beam/pull/28590#discussion_r1337550437 was valid. @kellen do you suggest revert this change or there is a viable forward fix ?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make SerializableConfiguration cacheable [beam]
Posted by "kellen (via GitHub)" <gi...@apache.org>.
kellen commented on PR #28590:
URL: https://github.com/apache/beam/pull/28590#issuecomment-1828473489
This PR has introduced a race between the setting of `serializationCache` and the write. We are seeing flaky tests in scio (tracking issue spotify/scio#5099)
I am not sure why the cache should be cleared on `get` or `readExternal`, but if that is necessary `serializationCache` accesses should be synchronized
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337416209
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
thanks. shall we invalidate the cache in readExternal also (or is there a way to read ObjectInput to both cache and conf) - where conf is re-constructed
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ddebowczyk92 commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "ddebowczyk92 (via GitHub)" <gi...@apache.org>.
ddebowczyk92 commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337511248
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
I don't think it's necessary because `readExternal` is used to deserialize `ObjectInput` into a `SerializableConfiguration` object. When this method is called (i.e., by invoking the `ObjectInputStream#readObject()` method), a newly created `SerializableConfiguration` instance is 'bare naked,' meaning that the `serializationCache` is empty.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] Abacn commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337550437
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
Sorry I may missed something. `readExternal` is used to deserialize ObjectInput into conf that handled by SerializableConfiguration. SerializableConfiguration itself may or may not have empty cache. If there is a writeExternal called prior to readExternal (regardless of caller, this technically could be the case) then the cache was not empty and should be invalidated.
Also, should we make serializationCache synchronized?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ddebowczyk92 commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "ddebowczyk92 (via GitHub)" <gi...@apache.org>.
ddebowczyk92 commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337635453
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
okay you are right, technically it's possible. I added cache invalidation in a PR update.
to be honest, I didn't consider making serializationCache synchronized, because the implementation of SerializableConfiguration isn't thread-safe. Anytime a value is assigned to serializationCache variable or set to null surrounded it could be surrounded by synchronization block. Or maybe you have other idea on your mind?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ddebowczyk92 commented on a diff in pull request #28590: Make SerializableConfiguration cacheable
Posted by "ddebowczyk92 (via GitHub)" <gi...@apache.org>.
ddebowczyk92 commented on code in PR #28590:
URL: https://github.com/apache/beam/pull/28590#discussion_r1337361747
##########
sdks/java/io/hadoop-common/src/main/java/org/apache/beam/sdk/io/hadoop/SerializableConfiguration.java:
##########
@@ -54,8 +58,15 @@ public Configuration get() {
@Override
public void writeExternal(ObjectOutput out) throws IOException {
+ if (serializationCache == null) {
Review Comment:
I think it's hard to identify all the places where `Configuration` could be modified after the serialization cache is initialized. We can assume that anytime `Configuration` is accessed by calling `SerializableConfiguration#get()` it might get modified therefore making `serializationCache` store expired/invalid data. To address this concern, I added cache invalidation at the beginning of the `SerializableConfiguration#get()` method in a PR update.
I tested this change on the same job I described in https://github.com/apache/beam/issues/28589 and it still works
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #28590: Make SerializableConfiguration cacheable
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28590:
URL: https://github.com/apache/beam/pull/28590#issuecomment-1729879893
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org