Unified Row Handling - geowave-985#1040
Conversation
rfecher
left a comment
There was a problem hiding this comment.
A few things to look into before we merge...
| /** | ||
| * Public access to AdapterStore | ||
| */ | ||
| public AdapterStore getAdapterStore(); |
There was a problem hiding this comment.
Methods internal to GeoWave I'd prefer to be outside of the DataStore interface...
DataStore is the main user facing API and should be as clean as possible, these methods are for relatively low-level mechanics used internally for reusability and would be better suited as abstract methods in BaseDataStore than exposed through the interface
There was a problem hiding this comment.
I'll have to look at it. I must've added that because I needed access to the adapter store via the interface.
There was a problem hiding this comment.
I will remove that, and change the method args to use BaseDataStore
| @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "private class only accessed internally") | ||
| public class GeowaveRowId implements | ||
| NativeGeoWaveRow | ||
| public class GeowaveRowId |
There was a problem hiding this comment.
Could this be replaced by GeoWaveRowImpl?
There was a problem hiding this comment.
well, actually I do see that GeoWaveRowImpl has the value and this doesn't...would it make sense for GeoWaveRowImpl to contain GeoWaveRowId to consolidate any of the row ID logic, and then GeoWaveRowImpl is then basically a container for GeoWaveRowId + value
There was a problem hiding this comment.
Yeah, GeoWaveRowImpl already manages the row ID. I did tackle many of the places where that (or AccumuloID, or whatever) were being used, but apparently not all of them. I need to go back and finish this up.
| return statsStore; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
unnecessary if we move decodeRow to BaseDataStore
| List<Integer> fieldPositions = BitmaskUtils.getFieldPositions(row.getFieldMask()); | ||
|
|
||
| // Collect the valid fields | ||
| int fieldIndex = 0; |
There was a problem hiding this comment.
hmm, the temporary hack still lives...I can take a look at it, I had thought it should go away if we have the field mask for all datastore
There was a problem hiding this comment.
decodeRow in DataStoreUtils isn't used anymore - forgot to delete it. However, the same logic is now in BaseDataStore's decodeRow, but it's not 'the hack' anymore, in that Cass & Dyn now have field masks. This code is just walking through the list of fields identified by the field mask to decide what to fields to include.
There was a problem hiding this comment.
Need to go back to HBase decodeRow as the template. This should allow us to unify properly and eliminate most of the duplication in the utils classes
There was a problem hiding this comment.
BaseDataStore's decodeRow is now sync'd up w/ HBase logic; however, we won't be able to completely integrate until we solve visibility handling.
| private final static Logger LOGGER = Logger.getLogger(CassandraDataStore.class); | ||
| public static final Integer PARTITIONS = 4; | ||
|
|
||
| static { |
There was a problem hiding this comment.
is this just meant as a temporary thing in your dev environment?
There was a problem hiding this comment.
Yep, sorry. I need to mark those up better when I do that.
b415359 to
e05cd14
Compare
No description provided.