tickets/DM-43384 #846

jgates108 · 2024-03-27T17:23:10Z

No description provided.

…tabase Also extended MySQL API to extract results from 5 columns.

iagaponenko · 2024-04-08T20:22:25Z

src/czar/CzarChunkMap.h

+    ChunkMapException(Context const& ctx, std::string const& msg) : util::Issue(ctx, msg) {}
+};
+
+class CzarChunkMap {


The class needs to be documented.

That is probably not the most recent commit that you are commenting on. 10+ lines of documentation.

I simply followed a link to the PR from the ticket. At a time when I was reviewing the PR I didn't see any other commits. Did you do git push -f on your recent mods?

I have found changes to the header in another commit titled:

Added unit test

That confused me since I followed the commit structure and expected that the class would be documented in a commit where the class was introduced.

Perhaps I should ignore the commit structure and review changes on the PR instead of commit-by-commit?

I could see the comments on the PR when I entered the previous message.

src/czar/CzarChunkMap.h

iagaponenko · 2024-04-08T20:29:15Z

src/czar/CzarChunkMap.h

+    public:
+        using Ptr = std::shared_ptr<ChunkData>;
+        ChunkData(int chunkId_) : _chunkId(chunkId_) {}
+        int64_t const _chunkId;


Public members of classes can't start with _. The same problem seems to exist for all members of both classes: ChunkData and WorkerChunksData.

The second comment is regarding a choice of type for chunk numbers. Chunks are small positive numbers starting with 0. Values of the numbers do not exceed 1 million. I can't even see a scenario when the chunk number would be 1 billion. The right type here would be unsigned int or uint32_t.

iagaponenko · 2024-04-08T20:34:16Z

src/qmeta/QMetaMysql.cc

+/* &&&
+QMeta::ChunkMap QMetaMysql::getChunkMap() { // &&&
+    lock_guard<mutex> sync(_dbMutex);
+>>>>>>> 07d082050 (Added code to read chunk disposition map and organize for czar use.)


Is this because of the unresolved conflict during rebase against the "feature branch"?

iagaponenko · 2024-04-08T20:55:01Z

src/czar/CzarChunkMap.h

+        void addToWorkerHasThis(std::shared_ptr<WorkerChunksData> const& worker);
+
+        /// Key is databaseName+tableName, value is size in bytes.
+        std::map<std::pair<std::string, std::string>, int64_t> _dbTableMap;


You may replace the generic std::pair with something more specific and friendlier to the code readers, and which would be easier to track in the rest of the code:

struct DatabaseTable { std::strng database; std::strng table; };

The second comment is that sizes in C++ are usually expressed using unsigned integer types std::size_t, std::uint64_t, etc. Negative values make no sense for the size.

std::pair works as a key in a map as long as both items are comparable. Struct can be done, but the comparison needs to be defined. I'm not sure it merits the effort here as database name + table name is a normal pairing for database tables.

iagaponenko · 2024-04-08T21:03:43Z

src/czar/CzarChunkMap.h

+    class WorkerChunksData {
+    public:
+        using Ptr = std::shared_ptr<WorkerChunksData>;
+        WorkerChunksData(std::string const& wId) : _workerId(wId) {}


This isn't very important. However, it's always easier to read a code where there is a direct mapping of the construction parameters to the corresponding members like in:

WorkerChunksData(std::string const& workerId) : _workerId(workerId) {}

...unless you're going to have the public/protected method like:

std::string const& workerId() const { return _workerId; }

In the latter case, the following usually works well:

WorkerChunksData(std::string const& workerId_) : _workerId(workerId_) {}

Changed name to workerId.

iagaponenko · 2024-04-08T21:06:35Z

src/czar/CzarChunkMap.h

+
+        /// Map of chunks this worker will handle during shared scans.
+        std::map<int, ChunkData::Ptr> _sharedScanChunkMap;
+        int64_t _sharedScanTotalSize = 0;


Size is usually expressed using unsigned integer types.

iagaponenko · 2024-04-08T21:15:21Z

src/czar/CzarChunkMap.h

+
+    class WorkerChunksData;
+
+    class ChunkData {


Having a public nested class that has all its members public may be a big problem. The same comment applies to the relevant class WorkerChunksData. You allow any user of the class CzarChunkMap to mess with the internal state of CzarChunkMap as well as with internal states of ChunkData and WorkerChunksData. This design violates many key principles of the Object-oriented programming.

All of the members are private with no method to change their values. Aside from calcTotalBytes, which I'm making private now.

CzarChunkMap can only set the values because it a friend to this class. Nothing else should be able to alter these. This applies to all nested classes in CzarChunkMaps.

The underlying issue is that these maps will be used heavily. Making copies or locking mutexes will be expensive, so the nested classes need to be exposed for rapid cheap access, but need to be read only for safety. The friend class seemed to be the best way to do that. const isn't really an option for _totalBytes or _primaryScanWorker.

Yes, I can see it now after reviewing other changes made to these classes in the second commit:

Added unit test.

So, please, disregard my comments.

iagaponenko · 2024-04-08T21:19:27Z

src/czar/CzarChunkMap.cc

+        return false;
+    }
+
+    auto const& jsChunks = qChunkMap.chunks;


I thought the chunk map was migrated away from the JSON library.

iagaponenko

Looks good

iagaponenko · 2024-04-08T23:11:45Z

src/czar/CzarChunkMap.h

+    ChunkMapException(Context const& ctx, std::string const& msg) : util::Issue(ctx, msg) {}
+};
+
+class CzarChunkMap {


I have found changes to the header in another commit titled:

Added unit test

That confused me since I followed the commit structure and expected that the class would be documented in a commit where the class was introduced.

Perhaps I should ignore the commit structure and review changes on the PR instead of commit-by-commit?

iagaponenko · 2024-04-09T00:08:08Z

src/czar/CzarChunkMap.h

+
+    class WorkerChunksData;
+
+    class ChunkData {


Yes, I can see it now after reviewing other changes made to these classes in the second commit:

Added unit test.

So, please, disregard my comments.

jgates108 · 2024-04-09T16:16:54Z

This PR was made before the tickets/DM-43715 was created. Since that branch is still the same as main, the changes in the PR are the same, but this seems to have other effects on the PR.

jgates108 force-pushed the tickets/DM-43384 branch from 12ddeeb to 0a63ea8 Compare March 27, 2024 17:50

iagaponenko and others added 8 commits April 5, 2024 20:31

Extended schema of qmeta to support persistent chunk map

e0fc435

Extended transient API of QMeta to read workers-to-chunks map from da…

f3c46cb

…tabase Also extended MySQL API to extract results from 5 columns.

Extended Replication Controller to update chunk map at Czar

1010539

Merge branch 'tickets/DM-43603' into tickets/DM-43715

2c52584

Added code to read chunk disposition map and organize for czar use.

c46224a

Added unit test.

f908d6b

Made failure to create CzarChunkMaps a non-fatal error.

71acedf

Changed code to use QMeta::ChunkMap instead of json object.

23e0bcd

jgates108 force-pushed the tickets/DM-43384 branch from a58f49c to 23e0bcd Compare April 8, 2024 20:01

iagaponenko reviewed Apr 8, 2024

View reviewed changes

iagaponenko approved these changes Apr 9, 2024

View reviewed changes

Changes for review.

fa63d49

fritzm closed this Aug 5, 2024

fritzm deleted the tickets/DM-43384 branch August 5, 2024 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tickets/DM-43384 #846

tickets/DM-43384 #846

jgates108 commented Mar 27, 2024

iagaponenko Apr 8, 2024

jgates108 Apr 8, 2024

iagaponenko Apr 8, 2024

iagaponenko Apr 8, 2024

jgates108 Apr 8, 2024

iagaponenko Apr 8, 2024

iagaponenko Apr 8, 2024

iagaponenko Apr 8, 2024

jgates108 Apr 8, 2024

iagaponenko Apr 8, 2024

jgates108 Apr 9, 2024

iagaponenko Apr 8, 2024

jgates108 Apr 9, 2024

iagaponenko Apr 8, 2024

jgates108 Apr 8, 2024

iagaponenko Apr 9, 2024

iagaponenko Apr 8, 2024

iagaponenko left a comment

iagaponenko Apr 8, 2024

iagaponenko Apr 9, 2024

jgates108 commented Apr 9, 2024

tickets/DM-43384 #846

tickets/DM-43384 #846

Conversation

jgates108 commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iagaponenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgates108 commented Apr 9, 2024