You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/03/10 03:16:30 UTC
[GitHub] [incubator-iceberg] chenjunjiedada opened a new pull request #833:
make fieldIds in SchemaVisitor thread safe
chenjunjiedada opened a new pull request #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833
This fixes https://github.com/apache/incubator-iceberg/issues/831.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #833: make fieldIds
in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-597200427
I don't think that the right solution to concurrent access is to use a concurrent queue. That avoids the exception you're seeing, but doesn't mean that the code is correct.
It looks like the problem is that a shared `SchemaVisitor`, in this case `MappingUtil.CreateMapping.INSTANCE`, has multiple threads updating the visitor's internal field ID stack. Since that stack is specific to the schema that is visited, it isn't correct to share the stack across threads. This was an oversight because the actual visitor doesn't keep any state so it seemed safe to share one instead of creating a new one each time.
The solution is to either avoid creating singleton `SchemaVisitor` instances, or to remove the field ID stack. I think we should remove the field ID stack, since it is only used when converting an Iceberg type to Avro (`TypeToSchema`).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada edited a comment on issue #833:
make fieldIds in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
chenjunjiedada edited a comment on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-598496532
I tried the way you provided, just have one concern that all other schema visitors are forced to add more stack frames, not sure if that could be optimized by the compiler.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #833: make fieldIds
in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-598430109
Ideally, this wouldn't create a new visitor for the one case that uses the field IDs. I opened a #840 with an alternative solution that uses new callbacks instead of a different visitor pattern. Could you look at that?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on issue #833: make
fieldIds in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-597728192
Thanks, I got it. Will update this later.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #833: make fieldIds
in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-597723370
@chenjunjiedada, you're right that we do need the id to produce the record name. That doesn't mean that we need to include the field id stack in _all_ of the schema visitors. A simple solution is to use a different schema visitor for the one case that needs the id.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #833: make fieldIds
in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-598810889
The solution in #840 won't add more stack frames because the calls are invoked and return outside the recursive call to `visit`.
I merged the other version so I'm going to close this. Thanks @chenjunjiedada!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on issue #833: make
fieldIds in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-597525476
Thanks @rdblue for the explanation.
I walked through the code path and found that we still need the `id` to for the record name since we need to do Avro resolution via name mapping when parsing the `ManifesEntry` and `ManifestFile`, which means we need to build a same name mapping (or with some renames) against the schema we define for `ManifesEntry` and `ManifestFile`. If we remove the `fieldIds` we need to find another way to do the same schema visiting logic that may be more complicated. Make sense?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on issue #833: make
fieldIds in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-598496532
I tried the way you provided, just have one concern that all other schema visitors are forced to add one more stack frame, not sure if that could be optimized by the compiler.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada edited a comment on issue #833:
make fieldIds in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
chenjunjiedada edited a comment on issue #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833#issuecomment-597525476
Thanks @rdblue for the explanation.
I walked through the code path and found that we still need the `id` to for the record name since we need to do Avro resolution via name mapping when parsing the `ManifesEntry` and `ManifestFile`, which means we need to build the same name mapping (or with some renames) against the schema we define for `ManifesEntry` and `ManifestFile`. If we remove the `fieldIds` stack from `SchemaVisitor` we need to find another way to do the same schema visiting logic that may be more complicated. So we may consider the first way, make sense?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue closed pull request #833: make fieldIds
in SchemaVisitor thread safe
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #833: make fieldIds in SchemaVisitor thread safe
URL: https://github.com/apache/incubator-iceberg/pull/833
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org