-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accumulo 1225 attempt2 #1
base: master
Are you sure you want to change the base?
Conversation
@@ -47,6 +50,12 @@ | |||
public class ZooCache { | |||
private static final Logger log = LoggerFactory.getLogger(ZooCache.class); | |||
|
|||
public final static Pattern TABLE_SETTING_CONFIG_PATTERN = | |||
Pattern.compile("(/accumulo/[0-9a-z-]+)(" + Constants.ZTABLES | |||
+ ")(/([a-zA-Z1-9]+|\\+r|!0|\\+rep))(" + Constants.ZTABLE_CONF + ")/table.*"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex for the table ID portion seems somewhat fragile as we may create more specialized tables in the future. Perhaps it should simply be ... Constants.ZTABLES + ")(/([^/]+))(" + Constants.ZTABLE_CONF ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you going to change this one?
Matcher tableConfigsMatcher = TABLE_CONFIGS_PATTERN.matcher(event.getPath()); | ||
if (tableConfigsMatcher.matches()) { | ||
try { | ||
byte[] configToRefresh = getZooKeeper().getData(event.getPath(), watcher, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that since we are going back to zookeeper to get the data, it could have been changed a second time and we will have missed the original property name that actually changed. Is it possible to get the changed value out of the event object instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivakegg, you make some good points that I am addressing in my next push to my branch. You can't get the changed value out of the WatchedEvent object. You do get the version out of the Stat object from a getData call on a Zookeeper object I will use track that version to make sure the value has not changed a second time. I will store the latest known table_configs node version in a class member variable in ZooCache to make sure we don't miss a NodeDataChanged event for that Znode.
byte[] configToRefresh = getZooKeeper().getData(event.getPath(), watcher, null); | ||
String strConfigToRefresh = new String(configToRefresh); | ||
remove(strConfigToRefresh); | ||
get(strConfigToRefresh); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, you are relying on the get() method to recreate the watcher on the config version node. I think we may want to reset the watcher in this method instead. This way we can handle resetting the watcher correctly on a NodeChildrenChanged, NodeCreated, or NodeDeleted event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are absolutely "resetting" the watcher here in the process method of ZCacheWatcher. Watches are triggered once and then it removed from ZooKeeper. Recreating and Resetting watches is synonymous in this context.
The "table-config-version" znode doesn't have children so you won't see NodeChildrenChanged event on it ever. It is created during initialization of the server and no watch can be put on it before that (in the ZooCache class at least) so you won't see a NodeCreated event on the "table-config-version" znode. We won't delete that node so you won't see a NodeDeleted event. We sort of of rig it to only see events on it during NodeDataChanged because we change the data in it ourselves in the ZooUtil. We do this the only time we care about and that is when a table config changes.
// The reset of the watcher for the above mentioned node will be done in the NodeDataChanged | ||
// condition of the ZCacheWatcher.process function when it has been triggered | ||
// by a change created in ZooUtil.putData. | ||
if (!tableConfigWatcherSet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never see this set back to false once the watcher was triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want it to set it back to false. This just gets the Watch on the "table-config-version" node the first time. I am trying to find a better way to put the watch on this node the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I thought you needed to reset the watcher every time it was triggered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the watcher for the table-config-version node gets reset in the process method when the NodeDataChanged event gets called by the watch on the table-config-version node due to its modification inside of ZooUtil.putData.
No table config znodes are ever watched. That is the specification.
if (!tableConfigWatcherSet) { | ||
if (configMatcher.matches()) { | ||
String pathPrefix = configMatcher.group(1); | ||
zooKeeper.exists(pathPrefix + Constants.ZTABLE_CONFIG_VERSION, watcher); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be moved into the watcher process event above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't go into the watcher process event or that event will never be called because the "table-config-version" will not have a "watcher" on it.
I think one of the ZooCache constructors is a create place to "put the watch" on "table-config-version" but I don't have a way to find out the accumulo instance ID there. That is why I have to "regex" it out of one of the table config zpaths when I get chance to.
I need to find a better way to put on the initial watch. Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get your reasoning. This method is called from the watcher process method. Hence whether this part of the routine is here or in the watcher process method itself should not matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I can make this modification to the code so we can get passed this issue. I will do it tomorrow.
I did a search of every where setData can be called. I found an instance in the ZooReaderWriter class. Have you verified that table configuration is never create/updated via the ZooReaderWriter mutate call? |
Thank you for finding that. I will look into whether that can occur. This is going well by the way. This all compiles in 'mvn clean verify -Psunny' . It runs well in an UNO instance. I can create and delete tables in the Accumulo shell. I can modify table configs in the Accumlo shell too. I can ingest data and clone tables with ingested data. We are on our way to a good PR here. |
Table configuration are managed by the TablePropUtil class. The static setTableProperty method in that class sets table configurations. I reviewed the call graph and caller graph for ZooReaderWriter.mutate and see no effect it has on table properties. Thanks for pointing out your concern. Glad I checked it. |
Do you want me to change that Regex? I don't know why we should. I will change it if you want. |
I made the change to the regex TABLE_SETTING_CONFIG_PATTERN. You were 100% right about the need for that. |
04fdeb6
to
7adbcce
Compare
|
||
switch (event.getType()) { | ||
case NodeDataChanged: | ||
Matcher tableConfigsMatcher = TABLE_CONFIGS_PATTERN.matcher(event.getPath()); | ||
if (tableConfigsMatcher.matches()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need to check for matches both outside and inside the processTableConfigurationItem method. choose one or the other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check it inside the function. Thank you. Good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a new version. I just got rid of the TABLE_CONFIGS_PATTERN and do
(event.getPath().endsWith(Constants.ZTABLE_CONFIG_VERSION))
in the case statement. Simpler and computationally less expensive.
lastTableConfigVersion = versionStat.getVersion(); | ||
refreshTableConfig(configToRefresh); | ||
if (log.isTraceEnabled()) { | ||
log.trace("Successfully refreshed table config (the most efficient way way): " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most efficient way way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change that to something like " by only refreshing the updated table configuration."
} | ||
} | ||
|
||
private void refreshTableConfig(byte[] configToRefresh) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add method comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Just pushed the new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
um.... I do not see a method comment here on refreshTableConfig.....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the method comment and made the others I wrote conform to the previously written comments in the class.
7adbcce
to
01b9562
Compare
} | ||
|
||
private static String getTablePath(String zkRoot, TableId tableId) { | ||
return zkRoot + Constants.ZTABLES + "/" + tableId.canonical() + Constants.ZTABLE_CONF; | ||
} | ||
|
||
private static void updateTableConfigTrackingZnode(String zPath, ZooReaderWriter zoo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need a method comment here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added the comment.
…all table configs
58a3ac4
to
1bf2541
Compare
…all table configs
8fb054a
to
9a6d726
Compare
No description provided.