Skip to content

sync_hirschberg_sinclair_second_edition#46

Open
alskvar wants to merge 7 commits into
krzysztof-turowski:masterfrom
alskvar:new_branch
Open

sync_hirschberg_sinclair_second_edition#46
alskvar wants to merge 7 commits into
krzysztof-turowski:masterfrom
alskvar:new_branch

Conversation

@alskvar

@alskvar alskvar commented Jan 27, 2024

Copy link
Copy Markdown

Everything together

@krzysztof-turowski krzysztof-turowski left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you very much - you probably need to read wider about the existing framework and its usage instead of reinventing infrastructure. So please follow other algorithms and conform to the general flow common to all of them.

Comment thread Makefile Outdated
go run example/leader_undirected_ring_sync_franklin.go 10
go run example/leader_undirected_ring_sync_prob_as_far.go 10
go run example/leader_undirected_ring_async_hirschberg_sinclair.go 10
go run example/leader_undirected_ring_async_hirschberg_sinclair_second_edition.go 10

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rename everything to leader_undirected_ring_async_hirschberg_sinclair_2

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread README.md Outdated
#### Synchronized undirected ring
1. Franklin algorithm
1. Hirschberg-Sinclair algorithm
1. Hirschberg-Sinclair algorithm2.0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You can remove it, since it's just a second implementation of the same algorithm

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

package main

import (
pathToCandidate "github.com/krzysztof-turowski/distributed-framework/leader/undirected_ring/sync_hirschberg_sinclair_second_edition"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove pathToCandidate, just sync_hirschberg_sinclair_2 and it's fine + move github.com packages below system packages (leaving one line skip between them)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +284 to +285
// Shuffle the array
//rand.Seed(time.Now().UnixNano())

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove it (or uncomment)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

writeChannel <- leader
}

//arr := generateShuffledArray(N)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please remove it

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

Comment on lines +219 to +221
wg.Add(N)
runner := func(candidate ICandidate) {
defer wg.Done()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please migrate to the synchronizer (located in lib/lib_synchronizer.go, built by functions in lib/lib_graph.go) instead of reinventing the wheel - at least you could ask before re-implementing it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

Comment on lines +233 to +238
var input1, input2 <-chan []byte
var output1, output2 chan<- []byte
input1 = prevOutput
output1 = prevInput
input2 = nextInput
output2 = nextOutput

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

input1, input2 := prevOutput, nextInput

Here it's irrelevant since the whole function is to be removed, but please apply the shorthand notation elsewhere

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Has been removed

Comment on lines +121 to +122
This pseudocode is more suitable for a solution based on maintaining references to neighboring processes. However, in the implementation I provided, solution stuck to to passing information through channels, requiring a slight modification in the code structure. Nevertheless the given pseudocode effectively reflects the algorithm's flow, as much, as it reduces the number of messages).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I suppose this could become irrelevant when you migrate to graph and synchronizer as it's implemented in the library.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Has been removed

Comment on lines +292 to +301
type randBit struct {
R *rand.Rand
}

func generateRandomBit() bool {
rB := randBit{
R: rand.New(rand.NewSource(time.Now().UTC().UnixNano())),
}
return rB.R.Intn(2) == 0
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I guess this won't be needed too.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Has been removed

pathToCandidate.BuildUndirectedRingAndRun(N, argsArr)
}

func checkN(N int) bool {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please put it directly in code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

2. Code fitted to framework rules
3. Removed unnecessary quote in README.md
4. Synchronizer has been skipped
…e-round Synchronizer implemented within the framework.
…simulating behavior of directed ring with "twisting" on base of undirected one.

2. Updated Makefile
3. Added quote in README.md
4. Added implementation of BuildRingWithOriginalTopology for possibility to accomplish task (1).
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