[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16904119#comment-16904119 ] stack edited comment on HBASE-22623 at 8/9/19 7:28 PM: --- bq. "...but what I'm trying to understand is why. " Because we were afraid to expose this fundamental for others to manipulate. WALEdit shows up in a bunch of contexts. There is the original write to the WAL; there is the WAL Replay; and there is replication-time. Any mis-accounting of Cells or sizes would mess us up. Debugging a cluster with missing or mis-accounted edits is a time sink. We were being conservative. A few more notes: * I'm late to the party. Sorry. Duo raises points I would have (but does a better job making his point). Anoop notes that there are inconsistencies in our CP API which come to light because Geoffrey has brought the spotlight; that is no surprise. Lets file issues at least for now while iron is hot. * This is a really good discussion. was (Author: stack): bq. "...but what I'm trying to understand is why. " Because we were afraid to expose this fundamental for others to manipulate. WALEdit shows up in a bunch of contexts. There is the original write to the WAL; there is the WAL Replay; and there is replication-time. Any mis-accounting of Cells or sizes would mess us up. Debugging a cluster with missing or mis-accounted edits is a time sink. We were being conservative. A few more notes: * I'm late to the party. Sorry. Duo raises points I would have. Anoop notes that there are inconsistencies in our CP API which come to light because Geoffrey has brought the spotlight. There is no surprise here. * This is a really good discussion. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16904119#comment-16904119 ] stack edited comment on HBASE-22623 at 8/9/19 7:25 PM: --- bq. "...but what I'm trying to understand is why. " Because we were afraid to expose this fundamental for others to manipulate. WALEdit shows up in a bunch of contexts. There is the original write to the WAL; there is the WAL Replay; and there is replication-time. Any mis-accounting of Cells or sizes would mess us up. Debugging a cluster with missing or mis-accounted edits is a time sink. We were being conservative. A few more notes: * I'm late to the party. Sorry. Duo raises points I would have. Anoop notes that there are inconsistencies in our CP API which come to light because Geoffrey has brought the spotlight. There is no surprise here. * This is a really good discussion. was (Author: stack): bq. "...but what I'm trying to understand is why. " Because we were afraid to expose this fundamental for others to manipulate. WALEdit shows up in a bunch of contexts. There is the original write to the WAL; there is the WAL Replay; and there is replication-time. Any mis-accounting of Cells or sizes would mess us up. Debugging a cluster with missing or mis-accounted edits is a time sink. We were being conservative. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902242#comment-16902242 ] Andrew Purtell edited comment on HBASE-22623 at 8/7/19 4:54 PM: Also for clarification I think we can include a WALEdit parameter in the hook even if it is read only. That would be better than nothing, and would not be dumb. Maybe we can just table this discussion because only the WALKey needs to be modified at this time. The rathole about whether or not WALedit is immutable can be addressed later. (Overly conservative in my opinion, but there have been multiple expressions of concern about this so my opinion may be in the minority.) was (Author: apurtell): Also for clarification I think we can include a WALEdit parameter in the hook even if it is read only. That would be better than nothing, and would not be dumb. Maybe we can just table this discussion because only the WALKey needs to be modified at this time. The rathole about whether or not WALedit is immutable can be addressed later. (Still kind of silly in my opinion, but there have been multiple expressions of concern about this so my opinion may be in the minority.) > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902234#comment-16902234 ] Andrew Purtell edited comment on HBASE-22623 at 8/7/19 4:46 PM: I have to agree. I can call System.exit() from a coprocessor. That doesn't mean it is a good idea. A necessary precondition for coprocessor API use is an understanding you are messing with internals. It's not meant for arbitrary user code or user development. We don't need to protect against dumb users. What we can/should protect is our ability to change internal interfaces, so if there is a concern there that would certainly be valid. However these classes - WALKey and WALEdit - are both already LP(coproc). We are discussing if an add() method should be available or not and going right to a maximally restrictive position which is in my opinion inappropriate given the context. Coprocessor interfaces are meant to change things on the line between internal-only and extensible. This JIRA seeks to make WAL objects more extensible for coprocessors, by giving coprocessors a chance to change them *once* before they are committed to the WAL, and this is motivated by a real world example waiting over in Phoenix. This is a carefully considered change and very small in scope. It is exactly what we have CP interfaces for elsewhere. I think [~Apache9]'s position is not reasonable given this context, but he is entitled to his opinion. I hope he will consider changing it, though. was (Author: apurtell): I have to agree. I can call System.exit() from a coprocessor. That doesn't mean it is a good idea. A necessary precondition for coprocessor API use is an understanding you are messing with internals. It's not meant for arbitrary user code or user development. We don't need to protect against dumb users. What we can/should protect is our ability to change internal interfaces, so if there is a concern there that would certainly be valid. However these classes - WALKey and WALEdit - are both already LP(coproc). We are discussing if an add() method should be available or not and going right to a maximally restrictive position which is in my opinion inappropriate given the context. Coprocessor interfaces are meant to change things on the line between internal-only and extensible. This JIRA seeks to make WAL objects more extensible for coprocessors, by giving coprocessors a change to change them *once* before they are committed to the WAL, and this is motivated by a real world example waiting over in Phoenix. This is a carefully considered change and very small in scope. It is exactly what we have CP interfaces for elsewhere. I think [~Apache9]'s position is not reasonable given this context, but he is entitled to his opinion. I hope he will consider changing it, though. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902234#comment-16902234 ] Andrew Purtell edited comment on HBASE-22623 at 8/7/19 4:45 PM: I have to agree. I can call System.exit() from a coprocessor. That doesn't mean it is a good idea. A necessary precondition for coprocessor API use is an understanding you are messing with internals. It's not meant for arbitrary user code or user development. We don't need to protect against dumb users. What we can/should protect is our ability to change internal interfaces, so if there is a concern there that would certainly be valid. However these classes - WALKey and WALEdit - are both already LP(coproc). We are discussing if an add() method should be available or not and going right to a maximally restrictive position which is in my opinion inappropriate given the context. Coprocessor interfaces are meant to change things on the line between internal-only and extensible. This JIRA seeks to make WAL objects more extensible for coprocessors, by giving coprocessors a change to change them *once* before they are committed to the WAL, and this is motivated by a real world example waiting over in Phoenix. This is a carefully considered change and very small in scope. It is exactly what we have CP interfaces for elsewhere. I think [~Apache9]'s position is not reasonable given this context, but he is entitled to his opinion. I hope he will consider changing it, though. was (Author: apurtell): I have to agree. I can call System.exit() from a coprocessor. That doesn't mean it is a good idea. A necessary precondition for coprocessor API use is an understanding you are messing with internals. It's not meant for arbitrary user code or user development. We don't need to protect against dumb users. What we can/should protect is our ability to change internal interfaces, so if there is a concern there that would certainly be valid. However these classes - WALKey and WALEdit are both already LP(coproc). I think [~Apache9]'s position is not reasonable, but he is entitled to his opinion. I hope he will consider changing it, though. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16902234#comment-16902234 ] Andrew Purtell edited comment on HBASE-22623 at 8/7/19 4:42 PM: I have to agree. I can call System.exit() from a coprocessor. That doesn't mean it is a good idea. A necessary precondition for coprocessor API use is an understanding you are messing with internals. It's not meant for arbitrary user code or user development. We don't need to protect against dumb users. What we can/should protect is our ability to change internal interfaces, so if there is a concern there that would certainly be valid. However these classes - WALKey and WALEdit are both already LP(coproc). I think [~Apache9]'s position is not reasonable, but he is entitled to his opinion. I hope he will consider changing it, though. was (Author: apurtell): I have to agree. I can call System.exit() from a coprocessor. That doesn't mean it is a good idea. A necessary precondition for coprocessor API use is an understanding you are messing with internals. It's not meant for arbitrary user code or user development. We don't need to protect against dumb users. What we can/should protect is our ability to change LP(coproc) interfaces, so if there is a concern there that would certainly be valid. However these classes - WALKey and WALEdit are both already LP(coproc). I think [~Apache9]'s position is not reasonable, but he is entitled to his opinion. I hope he will consider changing it, though. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901668#comment-16901668 ] Geoffrey Jacoby edited comment on HBASE-22623 at 8/7/19 3:39 AM: - [~Apache9] - that seems like an argument that the new coproc hook shouldn't fire for "meta" WALEdits, just ones associated with user operations, rather than an argument against not exposing mutable WALEdit to coprocs in general. If we were to take that approach, would that just require a check to WALEdit.isMetaEdit(), or can user and system level Cells be interleaved in the same Edit? To your second point, every single coprocessor hook in HBase can be used to "break the logic of HBase", by their very nature, because an evil coproc developer can say "Thread.sleep(Long.MAX_VALUE)", "System.exit(1)", or even start deleting WAL or data directories. An operator who runs a coprocessor is trusting the coproc to the same extent they trust HBase. IMO we should be making it easy to do useful things correctly, and harder to do bad things accidentally, but we're never going to make coprocessors as they exist now completely safe. was (Author: gjacoby): [~Apache9] - that seems like an argument that the new coproc hook shouldn't fire for "meta" WALEdits, just ones associated with user operations, rather than an argument against not exposing mutable WALEdit to coprocs in general. If we were to take that approach, would that just require a check to WALEdit.isMetaEdit(), or can user and system level Cells be interleaved in the same Edit? To your second point, every single coprocessor hook in HBase can be used to "break the logic of HBase", by their very nature, because an evil coproc developer can say "Thread.sleep(Long.MAX_VALUE)", "System.exit(1)", or even start deleting WAL or data directories. An operator who runs a coprocessor is trusting the coproc to the same extent they trust HBase. IMO we should be making it easy to do useful things correctly, and harder to do bad things accidentally, but we're never going to make coprocessors as they exist now "safe". > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901262#comment-16901262 ] Andrew Purtell edited comment on HBASE-22623 at 8/6/19 5:12 PM: The explicit intent of the new hook is to modify or decorate the objects going into the WAL. That is at least WALKey, but a WAL entry is built from two objects: the WALKey, and the WALEdit. The hook should address both, otherwise we are leaving an obvious gap. We have been in a position before where CP hooks needed to be modified because they didn't quite expose what was necessary for the CP application to modify or replace in order to achieve its aim. I think we can avoid a fairly obvious gap here. The second question is why a WALEdit is immutable. My guess is it is only because there has never been a need to modify one after construction. We don't need to change WALEdit, but one of the parameters of this hook should carry the WALEdit, if only to make its state visible to the coprocessor. That said, I have no objection to removing the annotation from WALEdit's add method. Coprocessors should be able to change state in the agreed upon scope. In this issue I see us trying to build consensus that WALKey and WALEdit are useful inputs to a coprocessor which wants to decorate a WAL entry just before commit. There is a real use case for this in Phoenix. (Change capture device.) Unless there is an objection I think we have that consensus per the three way conversation between Geoffrey, Anoop, and myself. Edit: If we agree to do this, then let us also annotate WALKey and WALEdit as LP(Coproc) was (Author: apurtell): The explicit intent of the new hook is to modify or decorate the objects going into the WAL. That is at least WALKey, but a WAL entry is built from two objects: the WALKey, and the WALEdit. The hook should address both, otherwise we are leaving an obvious gap. We have been in a position before where CP hooks needed to be modified because they didn't quite expose what was necessary for the CP application to modify or replace in order to achieve its aim. I think we can avoid a fairly obvious gap here. The second question is why a WALEdit is immutable. My guess is it is only because there has never been a need to modify one after construction. We don't need to change WALEdit, but one of the parameters of this hook should carry the WALEdit, if only to make its state visible to the coprocessor. That said, I have no objection to removing the annotation from WALEdit's add method. Coprocessors should be able to change state in the agreed upon scope. In this issue I see us trying to build consensus that WALKey and WALEdit are useful inputs to a coprocessor which wants to decorate a WAL entry just before commit. There is a real use case for this in Phoenix. (Change capture device.) Unless there is an objection I think we have that consensus per the three way conversation between Geoffrey, Anoop, and myself. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901262#comment-16901262 ] Andrew Purtell edited comment on HBASE-22623 at 8/6/19 4:43 PM: The explicit intent of the new hook is to modify or decorate the objects going into the WAL. That is at least WALKey, but a WAL entry is built from two objects: the WALKey, and the WALEdit. The hook should address both, otherwise we are leaving an obvious gap. We have been in a position before where CP hooks needed to be modified because they didn't quite expose what was necessary for the CP application to modify or replace in order to achieve its aim. I think we can avoid a fairly obvious gap here. The second question is why a WALEdit is immutable. My guess is it is only because there has never been a need to modify one after construction. We don't need to change WALEdit, but one of the parameters of this hook should carry the WALEdit, if only to make its state visible to the coprocessor. That said, I have no objection to removing the annotation from WALEdit's add method. Coprocessors should be able to change state in the agreed upon scope. In this issue I see us trying to build consensus that WALKey and WALEdit are useful inputs to a coprocessor which wants to decorate a WAL entry just before commit. There is a real use case for this in Phoenix. (Change capture device.) Unless there is an objection I think we have that consensus per the three way conversation between Geoffrey, Anoop, and myself. was (Author: apurtell): The explicit intent of the new hook is to modify or decorate the objects going into the WAL. That is at least WALKey, but a WAL entry is built from two objects: the WALKey, and the WALEdit. The hook should address both, otherwise we are leaving an obvious gap. We have been in a position before where CP hooks needed to be modified because they didn't quite expose what was necessary for the CP application to change. I think we can avoid a fairly obvious gap here. The second question is why a WALEdit is immutable. My guess is it is only because there has never been a need to modify one after construction. We don't need to change WALEdit, but one of the parameters of this hook should carry the WALEdit, if only to make its state visible to the coprocessor. That said, I have no objection to removing the annotation from WALEdit's add method. Coprocessors should be able to change state in the agreed upon scope. In this issue I see us trying to build consensus that WALKey and WALEdit are useful inputs to a coprocessor which wants to decorate a WAL entry just before commit. There is a real use case for this in Phoenix. (Change capture device.) Unless there is an objection I think we have that consensus per the three way conversation between Geoffrey, Anoop, and myself. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16900145#comment-16900145 ] Geoffrey Jacoby edited comment on HBASE-22623 at 8/6/19 1:04 AM: - So, I see that the immutability of the Region-created WALEdit is an explicit goal of the current code, but what I'm trying to understand is _why_. Immutability can be a wonderful thing; it lets you write idempotent code along functional principles where methods have no side effects, just an output. But it seems to me the entire point of coprocessors _is to have side effects_. I'm sure we could come up with an indirect way to accomplish the adding of cells -- maybe something like WALEdit or List preWALAppend(WALKey, WALEditSuperInterfaceWithoutAddMethods edit), where HRegion then takes the returned new WALEdit or List and combines its Cells with the Cells of the existing WALEdit -- but I don't see what the added complexity achieves. It would just be more object churn for the garbage collector. Because coprocessors are embedded in core HBase logic, an operator is explicitly trusting the coproc author to get things right. It's good for the framework to make it easy to do the right thing, and guide people away from doing wrong or risky things. However, the messiest coproc-related bugs I've seen (both in Phoenix and some proprietary coprocs) have come from the hooks that are actually the most immutable: resource leaks from trying to recreate various types of scanners because setters aren't available. Not suggesting we change those hooks, just illustrating that immutable doesn't automatically mean "safe" or "easy" and mutable doesn't automatically mean "risky". It all depends on context. So I'm very curious what the context is here that makes the idea of changing the HRegion WALEdit directly risky, if changing it indirectly is OK. was (Author: gjacoby): So, I see that the immutability of the Region-created WALEdit is an explicit goal of the current code, but what I'm trying to understand is _why_. Immutability can be a wonderful thing; it lets you write idempotent code along functional principles where code has no side effects, just an output. But it seems to me the entire point of coprocessors _is to have side effects_. I'm sure we could come up with an indirect way to accomplish the adding of cells -- maybe something like WALEdit preWALAppend(WALKey, WALEditSuperInterfaceWithoutAddMethods edit), where HRegion then takes the returned new WALEdit and combines its Cells with the Cells of the existing WALEdit -- but I don't see what the added complexity achieves. It would just be more object churn for the garbage collector. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16900506#comment-16900506 ] Andrew Purtell edited comment on HBASE-22623 at 8/6/19 12:59 AM: - bq. WALEdit preWALAppend(WALKey, WALEditSuperInterfaceWithoutAddMethods edit) :-) I don't think immutable WALedits buy us anything, but I was not the one who did the refactor of the WAL interface so don't have the context. [~busbey] might you point us in the right direction? In my opinion we decorate WALedits with special cells, the compaction marker etc., and a coprocessor may want to do this too. Although it does not do so today I am thinking especially of Phoenix. We did consider special cells instead of WALkey attributes for HBASE-22622. Turns out special cells wouldn't work there but, now that we are adding a hook where a coprocessor can get control to decorate the key, we should include coverage for decorating/mutating the edit as well, or I fear this will be a miss later when someone needs it. was (Author: apurtell): bq. WALEdit preWALAppend(WALKey, WALEditSuperInterfaceWithoutAddMethods edit) :-) I don't think immutable WALedits buy us anything, but I was not the one who did the refactor of the WAL interface so don't have the context. [~busbey] might you point us in the right direction? In my opinion we decorate WALedits with special cells, the compaction marker etc., and a coprocessor may want to do this too. Although it does not do so today I am thinking especially of Phoenix. We did consider special cells instead of WALkey attributes for HBASE-22622. Now that we are adding a hook where a coprocessor can get control to decorate the key, we should include coverage for decorating/mutating the edit as well, or I fear this will be a miss later when someone needs it. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16900506#comment-16900506 ] Andrew Purtell edited comment on HBASE-22623 at 8/6/19 12:57 AM: - bq. WALEdit preWALAppend(WALKey, WALEditSuperInterfaceWithoutAddMethods edit) :-) I don't think immutable WALedits buy us anything, but I was not the one who did the refactor of the WAL interface so don't have the context. [~busbey] might you point us in the right direction? In my opinion we decorate WALedits with special cells, the compaction marker etc., and a coprocessor may want to do this too. Although it does not do so today I am thinking especially of Phoenix. We did consider special cells instead of WALkey attributes for HBASE-22622. Now that we are adding a hook where a coprocessor can get control to decorate the key, we should include coverage for decorating/mutating the edit as well, or I fear this will be a miss later when someone needs it. was (Author: apurtell): bq. WALEdit preWALAppend(WALKey, WALEditSuperInterfaceWithoutAddMethods edit) :-) I don't think immutable WALedits buy us anything, but I was not the one who did the refactor of the WAL interface so don't have the context. [~busbey] might you point us in the right direction? In my opinion we decorate WALedits with special cells, the compaction marker etc., and a coprocessor may want to do this too. Although it does not do so today I am thinking especially of Phoenix. We did consider special cells instead of WALkey attributes for HBASE-22622. Now that we are adding a hook for decorating the key, we should include coverage for decorating/mutating the edit as well, or I fear this will be a miss later when someone needs it. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[
https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886408#comment-16886408
]
Andrew Purtell edited comment on HBASE-22623 at 7/16/19 7:43 PM:
-
{quote}The signatures for the Append/Increment coprocs and the Batch mutation
coprocs are different enough that I'm somewhat skeptical we could make a single
hook that would work for both
{quote}
So don't modify existing hooks.
The original proposal on this issue was a new hook at every call site of
WAL#append in HRegion: "Add RegionObserver coprocessor hook for preWALAppend"
was (Author: apurtell):
{quote}The signatures for the Append/Increment coprocs and the Batch mutation
coprocs are different enough that I'm somewhat skeptical we could make a single
hook that would work for both
{quote}
So don't.
The original proposal on this issue was a new hook at every call site of
WAL#append in HRegion: "Add RegionObserver coprocessor hook for preWALAppend"
> Add RegionObserver coprocessor hook for preWALAppend
>
>
> Key: HBASE-22623
> URL: https://issues.apache.org/jira/browse/HBASE-22623
> Project: HBase
> Issue Type: New Feature
>Reporter: Geoffrey Jacoby
>Assignee: Geoffrey Jacoby
>Priority: Major
> Fix For: 3.0.0, 1.5.0, 2.3.0
>
>
> While many coprocessor hooks expose the WALEdit to implementing coprocs,
> there aren't any that expose the WALKey before it's created and added to the
> WALEntry.
> It's sometimes useful for coprocessors to be able to edit the WALKey, for
> example to add extended attributes using the fields to be added in
> HBASE-22622.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[
https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886408#comment-16886408
]
Andrew Purtell edited comment on HBASE-22623 at 7/16/19 7:41 PM:
-
{quote}The signatures for the Append/Increment coprocs and the Batch mutation
coprocs are different enough that I'm somewhat skeptical we could make a single
hook that would work for both
{quote}
So don't.
The original proposal on this issue was a new hook at every call site of
WAL#append in HRegion: "Add RegionObserver coprocessor hook for preWALAppend"
was (Author: apurtell):
{quote}The signatures for the Append/Increment coprocs and the Batch mutation
coprocs are different enough that I'm somewhat skeptical we could make a single
hook that would work for both
{quote}
So don't.
The proposal is a new hook at every call site of WAL#append in HRegion: "Add
RegionObserver coprocessor hook for preWALAppend"
> Add RegionObserver coprocessor hook for preWALAppend
>
>
> Key: HBASE-22623
> URL: https://issues.apache.org/jira/browse/HBASE-22623
> Project: HBase
> Issue Type: New Feature
>Reporter: Geoffrey Jacoby
>Assignee: Geoffrey Jacoby
>Priority: Major
> Fix For: 3.0.0, 1.5.0, 2.3.0
>
>
> While many coprocessor hooks expose the WALEdit to implementing coprocs,
> there aren't any that expose the WALKey before it's created and added to the
> WALEntry.
> It's sometimes useful for coprocessors to be able to edit the WALKey, for
> example to add extended attributes using the fields to be added in
> HBASE-22622.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[
https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886408#comment-16886408
]
Andrew Purtell edited comment on HBASE-22623 at 7/16/19 7:40 PM:
-
{quote}The signatures for the Append/Increment coprocs and the Batch mutation
coprocs are different enough that I'm somewhat skeptical we could make a single
hook that would work for both
{quote}
So don't.
The proposal is a new hook at every call site of WAL#append in HRegion: "Add
RegionObserver coprocessor hook for preWALAppend"
was (Author: apurtell):
{quote}The signatures for the Append/Increment coprocs and the Batch mutation
coprocs are different enough that I'm somewhat skeptical we could make a single
hook that would work for both
{quote}
So don't.
The proposal is a new hook at every call site of WAL#append: "Add
RegionObserver coprocessor hook for preWALAppend"
> Add RegionObserver coprocessor hook for preWALAppend
>
>
> Key: HBASE-22623
> URL: https://issues.apache.org/jira/browse/HBASE-22623
> Project: HBase
> Issue Type: New Feature
>Reporter: Geoffrey Jacoby
>Assignee: Geoffrey Jacoby
>Priority: Major
> Fix For: 3.0.0, 1.5.0, 2.3.0
>
>
> While many coprocessor hooks expose the WALEdit to implementing coprocs,
> there aren't any that expose the WALKey before it's created and added to the
> WALEntry.
> It's sometimes useful for coprocessors to be able to edit the WALKey, for
> example to add extended attributes using the fields to be added in
> HBASE-22622.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[
https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886361#comment-16886361
]
Andrew Purtell edited comment on HBASE-22623 at 7/16/19 6:20 PM:
-
{quote}They're mostly unrelated changes though, so it would either work to do
them together in this JIRA or separately.
{quote}
I don't follow that. My understanding of the intent here is to intercept
control of execution just prior to append of WAL entries to the log for
potential modification or decoration. If the solution misses some code paths
doesn't it fall short of the goal?
When I looked at this, I thought a new hook to be called prior to WAL#append at
all appropriate call sites would provide full coverage.
Wouldn't an application processing WALedits produced by Put, Delete, etc. also
need to process ones produced by Increment. Does Phoenix use Increments?
was (Author: apurtell):
{quote}They're mostly unrelated changes though, so it would either work to do
them together in this JIRA or separately.
{quote}
I don't follow that. My understanding of the intent here is to intercept
control of execution just prior to append of WAL entries to the log for
potential modification or decoration. If the solution misses some code paths
doesn't it fall short of the goal?
> Add RegionObserver coprocessor hook for preWALAppend
>
>
> Key: HBASE-22623
> URL: https://issues.apache.org/jira/browse/HBASE-22623
> Project: HBase
> Issue Type: New Feature
>Reporter: Geoffrey Jacoby
>Assignee: Geoffrey Jacoby
>Priority: Major
> Fix For: 3.0.0, 1.5.0, 2.3.0
>
>
> While many coprocessor hooks expose the WALEdit to implementing coprocs,
> there aren't any that expose the WALKey before it's created and added to the
> WALEntry.
> It's sometimes useful for coprocessors to be able to edit the WALKey, for
> example to add extended attributes using the fields to be added in
> HBASE-22622.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886347#comment-16886347 ] Geoffrey Jacoby edited comment on HBASE-22623 at 7/16/19 5:57 PM: -- I think Append and Increment use a different code path (see HRegion.doDelta) and so would have to be instrumented a different way, perhaps with extra coprocessor hooks, one for Increment and one for Append. (The existing Append and Increment pre-hooks don't take a convenient "carrier" object like the batch mutations do, and already use their return values for other purposes.) checkAndXXX seems to eventually go down the same batch mutation path as Puts and Deletes (see HRegion:4287) so the MiniBatchOperationInProgress solution above would work for them. They're mostly unrelated changes though, so it would either work to do them together in this JIRA or separately. was (Author: gjacoby): I think Append and Increment use a different code path (see HRegion.doDelta) and so would have to be instrumented a different way, perhaps with extra coprocessor hooks, one for Increment and one for Append. (The existing Append and Increment pre-hooks don't take a convenient "carrier" object like the batch mutations do, and already use their return values for other purposes.) checkAndXXX seems to eventually go down the same batch mutation path (see HRegion:4287) so the MiniBatchOperationInProgress solution above would work for them. They're mostly unrelated changes though, so it would either work to do them together in this JIRA or separately. > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886347#comment-16886347 ] Geoffrey Jacoby edited comment on HBASE-22623 at 7/16/19 5:55 PM: -- I think Append and Increment use a different code path (see HRegion.doDelta) and so would have to be instrumented a different way, perhaps with extra coprocessor hooks, one for Increment and one for Append. (The existing Append and Increment pre-hooks don't take a convenient "carrier" object like the batch mutations do, and already use their return values for other purposes.) checkAndXXX seems to eventually go down the same batch mutation path (see HRegion:4287) so the MiniBatchOperationInProgress solution above would work for them. They're mostly unrelated changes though, so it would either work to do them together in this JIRA or separately. was (Author: gjacoby): I think Append and Increment use a different code path (see HRegion.doDelta) and so would have to be instrumented a different way, perhaps with extra coprocessor hooks, one for Increment and one for Append. (The existing Append and Increment pre-hooks don't take a convenient "carrier" object like the batch mutations do, and already use their return values for other purposes.) checkAndXXX seems to eventually go down the same batch mutation path (see HRegion:4287) so the MiniBatchOperationInProgress solution above would work for them. They're unrelated changes though, so it > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > Fix For: 3.0.0, 1.5.0, 2.3.0 > > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (HBASE-22623) Add RegionObserver coprocessor hook for preWALAppend
[ https://issues.apache.org/jira/browse/HBASE-22623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16880747#comment-16880747 ] Geoffrey Jacoby edited comment on HBASE-22623 at 7/8/19 10:08 PM: -- As an alternative to adding yet-another-new-coprocessor hook, or changing the behavior of an old one, how about this? Complete HBASE-18127, which adds a notion of OperationContext to the ObserverContext for the mutation pipeline. Then add a new field to the OperationContext, WALAttributeMap, which would then allow for _any_ coprocessor hook (before WAL committal) to annotate the WALKey. Then the WAL writing code would just read from the OperationContext's WALAttributeMap and annotate the WALKey appropriately. No publicly overridable code would ever get anywhere near a protobuf, and the only interface that would change would be the HBASE-18127 OperationContext one that has never been released. (This would leave as future work the more general cases of HBASE-18127, such as passing information from a RegionObserver to a WALObserver, which prevents use of thread local storage.) [~apurtell] [~anoop.hbase][~abhishek.chouhan][~stack] was (Author: gjacoby): As an alternative to adding yet-another-new-coprocessor hook, or changing the behavior of an old one, how about this? Complete HBASE-18127, which adds a notion of OperationContext to the ObserverContext for the mutation pipeline. Then add a new field to the OperationContext, WALAttributeMap, which would then allow for _any_ coprocessor hook (before WAL committal) to annotate the WALKey. Then the WAL writing code would just read from the OperationContext and annotate the WALKey appropriately. No publicly overridable code would ever get anywhere near a protobuf, and the only interface that would change would be the HBASE-18127 OperationContext one that has never been released. (This would leave as future work the more general cases of HBASE-18127, such as passing information from a RegionObserver to a WALObserver, which prevents use of thread local storage.) [~apurtell] [~anoop.hbase][~abhishek.chouhan][~stack] > Add RegionObserver coprocessor hook for preWALAppend > > > Key: HBASE-22623 > URL: https://issues.apache.org/jira/browse/HBASE-22623 > Project: HBase > Issue Type: New Feature >Reporter: Geoffrey Jacoby >Assignee: Geoffrey Jacoby >Priority: Major > > While many coprocessor hooks expose the WALEdit to implementing coprocs, > there aren't any that expose the WALKey before it's created and added to the > WALEntry. > It's sometimes useful for coprocessors to be able to edit the WALKey, for > example to add extended attributes using the fields to be added in > HBASE-22622. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
