Skip to content

Conversation

dlmarion
Copy link
Contributor

This change reverts commits 242438a from #5641
and 963150d from #5645 and fixes the root
cause of the issues, which was that the scanref and fate tables were not
being created during the upgrade.

dlmarion added 3 commits June 17, 2025 17:55
This change reverts commits 242438a from apache#5641
and 963150d from apache#5645 and fixes the root
cause of the issues, which was that the scanref and fate tables were not
being created during the upgrade.
@dlmarion dlmarion self-assigned this Jun 17, 2025
@dlmarion dlmarion changed the title Fix upgrade table creation Reverted 5641,5645. Fix root cause of upgrade error Jun 17, 2025
@dlmarion dlmarion added this to the 4.0.0 milestone Jun 17, 2025
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, seems fine. This would be easier once we collapse the two upgraders for 4.0 into a single one. Some stuff could be simplified and inlined.

Comment on lines +404 to +409
expect(zrw.putPersistentData(isA(String.class), isA(byte[].class), isA(NodeExistsPolicy.class)))
.andReturn(true).times(7);
expect(context.getPropStore()).andReturn(store).atLeastOnce();
expect(store.exists(isA(TablePropKey.class))).andReturn(false).atLeastOnce();
store.create(isA(TablePropKey.class), isA(Map.class));
expectLastCall().once();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably use anyObject() for these parameters. It would avoid the need to suppress the warning, I think. Or, you could get more specific, and use eq() to pass specific objects. For the Array, you could pass a more specific matcher.

At the very least, a comment to explain where the count of 7 comes from would be useful (3 for name/conf/status for 2 tables, plus 1 for the mapping update? or something like that)

Namespace.ACCUMULO.id(), SystemTables.FATE.tableName(), TableState.ONLINE,
ZooUtil.NodeExistsPolicy.FAIL);
} catch (InterruptedException | KeeperException ex) {
Thread.currentThread().interrupt();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to re-set the interrupted flag here, because you're exiting the current thread in the next line with the ISE. I'm also wondering if this boilerplate exception handling can be pushed into the preparePre4_0NewTableState method, so we can just inline this method. The namespace, nodeexistspolicy, and online state can probably all be inlined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants